feat: add manage_profiler and reserialize_assets tools#827
Open
youngwoocho02 wants to merge 3 commits intoCoplayDev:betafrom
Open
feat: add manage_profiler and reserialize_assets tools#827youngwoocho02 wants to merge 3 commits intoCoplayDev:betafrom
youngwoocho02 wants to merge 3 commits intoCoplayDev:betafrom
Conversation
… profiler state Adds a new built-in tool that reads Unity Profiler frame data directly via ProfilerDriver, providing the same data visible in the Profiler window. Supports reading multiple frames, filtering by thread/sample name/minimum time, and controlling profiler state (enable/disable/clear). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Forces Unity to reserialize assets to the current serialization format. Supports single path or array of paths. Useful after Unity upgrades, script changes affecting serialized data, or .meta regeneration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Reviewer's GuideAdds two new Unity editor tools, Sequence diagram for manage_profiler tool invocationsequenceDiagram
actor Developer
participant MCPServer
participant ManageProfilerToolPython
participant UnityTransport
participant UnityEditor
participant ManageProfilerCs
Developer->>MCPServer: Invoke manage_profiler(action=read_frames, frame_count, thread, filter, min_ms)
MCPServer->>ManageProfilerToolPython: Call async manage_profiler(ctx, params)
ManageProfilerToolPython->>ManageProfilerToolPython: Build params dict with action and optional frameCount, thread, filter, minMs
ManageProfilerToolPython->>UnityTransport: send_with_unity_instance(async_send_command_with_retry, unity_instance, manage_profiler, params)
UnityTransport->>UnityEditor: Send MCP command manage_profiler with params
UnityEditor->>ManageProfilerCs: HandleCommand(@params as JObject)
ManageProfilerCs->>ManageProfilerCs: Validate @params not null
ManageProfilerCs->>ManageProfilerCs: Create ToolParams from JObject
ManageProfilerCs->>ManageProfilerCs: GetRequired action and switch on action
ManageProfilerCs->>ManageProfilerCs: ReadFrames(ToolParams)
ManageProfilerCs->>ManageProfilerCs: Use ProfilerDriver to read recent frames and samples
ManageProfilerCs-->>UnityEditor: SuccessResponse with frames data
UnityEditor-->>UnityTransport: Serialized SuccessResponse
UnityTransport-->>ManageProfilerToolPython: Response dict
ManageProfilerToolPython->>ManageProfilerToolPython: Normalize response to {success, message, data}
ManageProfilerToolPython-->>MCPServer: Dict result
MCPServer-->>Developer: Tool result with profiler samples
Sequence diagram for reserialize_assets tool invocationsequenceDiagram
actor Developer
participant MCPServer
participant ReserializeAssetsToolPython
participant UnityTransport
participant UnityEditor
participant ReserializeAssetsCs
Developer->>MCPServer: Invoke reserialize_assets(path or paths)
MCPServer->>ReserializeAssetsToolPython: Call async reserialize_assets(ctx, path, paths)
ReserializeAssetsToolPython->>ReserializeAssetsToolPython: Build params dict with path or paths
ReserializeAssetsToolPython->>UnityTransport: send_with_unity_instance(async_send_command_with_retry, unity_instance, reserialize_assets, params)
UnityTransport->>UnityEditor: Send MCP command reserialize_assets with params
UnityEditor->>ReserializeAssetsCs: HandleCommand(@params as JObject)
ReserializeAssetsCs->>ReserializeAssetsCs: Validate @params not null
ReserializeAssetsCs->>ReserializeAssetsCs: Create ToolParams from JObject
ReserializeAssetsCs->>ReserializeAssetsCs: Read path and paths parameters into list
ReserializeAssetsCs->>ReserializeAssetsCs: Call AssetDatabase.ForceReserializeAssets(paths)
ReserializeAssetsCs-->>UnityEditor: SuccessResponse with reserialized paths
UnityEditor-->>UnityTransport: Serialized SuccessResponse
UnityTransport-->>ReserializeAssetsToolPython: Response dict
ReserializeAssetsToolPython->>ReserializeAssetsToolPython: Normalize response to {success, message, data}
ReserializeAssetsToolPython-->>MCPServer: Dict result
MCPServer-->>Developer: Tool result with reserialized assets info
Class diagram for new Unity editor tools and Python wrappersclassDiagram
class ManageProfiler {
<<static>>
+HandleCommand(JObject params) object
-ReadFrames(ToolParams p) object
-EnableProfiler() object
-DisableProfiler() object
-GetStatus() object
-ClearFrames() object
-FormatTime(float ms) string
}
class ReserializeAssets {
<<static>>
+HandleCommand(JObject params) object
}
class ToolParams {
}
class ErrorResponse {
}
class SuccessResponse {
}
class ProfilerDriver {
<<static>>
+bool enabled
+int firstFrameIndex
+int lastFrameIndex
+GetRawFrameDataView(int frameIndex, int threadIndex) RawFrameDataView
+ClearAllFrames() void
}
class RawFrameDataView {
+bool valid
+int sampleCount
+float GetSampleTimeMs(int index)
+string GetSampleName(int index)
+int GetSampleChildrenCount(int index)
}
class UnityProfilerStatic {
<<static>>
+bool enabled
}
class Application {
<<static>>
+bool isPlaying
}
class AssetDatabase {
<<static>>
+ForceReserializeAssets(List~string~ assetPaths) void
}
class ManageProfilerToolPython {
+manage_profiler(Context ctx, string action, int frame_count, int thread, string filter, float min_ms) dict~string, Any~
}
class ReserializeAssetsToolPython {
+reserialize_assets(Context ctx, string path, List~string~ paths) dict~string, Any~
}
class Context {
}
class UnityTransport {
+send_with_unity_instance(fn, unity_instance, string tool_name, dict~string, Any~ params) Any
}
class LegacyUnityConnection {
+async_send_command_with_retry(unity_instance, string tool_name, dict~string, Any~ params) Any
}
ManageProfiler ..> ToolParams : uses
ManageProfiler ..> ErrorResponse : returns
ManageProfiler ..> SuccessResponse : returns
ManageProfiler ..> ProfilerDriver : controls
ManageProfiler ..> RawFrameDataView : reads
ManageProfiler ..> UnityProfilerStatic : controls
ManageProfiler ..> Application : queries
ReserializeAssets ..> ToolParams : uses
ReserializeAssets ..> ErrorResponse : returns
ReserializeAssets ..> SuccessResponse : returns
ReserializeAssets ..> AssetDatabase : calls
ManageProfilerToolPython ..> Context : takes
ManageProfilerToolPython ..> UnityTransport : uses
ManageProfilerToolPython ..> LegacyUnityConnection : passes_to
ReserializeAssetsToolPython ..> Context : takes
ReserializeAssetsToolPython ..> UnityTransport : uses
ReserializeAssetsToolPython ..> LegacyUnityConnection : passes_to
UnityTransport ..> LegacyUnityConnection : wraps
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughAdds two Unity Editor tools (ManageProfiler, ReserializeAssets) with C# HandleCommand entry points, Python server wrappers to invoke them, and accompanying unit/integration tests. ManageProfiler supports read_frames, enable, disable, status, clear; ReserializeAssets forces asset reserialization from provided path(s). Changes
Sequence Diagram(s)sequenceDiagram
participant Server as Server (Python)
participant Unity as Unity Editor (ManageProfiler)
participant Profiler as Profiler API (ProfilerDriver)
Server->>Unity: send command { action: "read_frames", frame_count?, thread?, filter?, min_ms? }
activate Unity
Unity->>Profiler: query enabled / frame indices
Profiler-->>Unity: enabled? / firstFrame / lastFrame
alt profiler enabled
Unity->>Profiler: request frame data for range
Profiler-->>Unity: frames + sample data
Unity->>Unity: filter by min_ms and optional name filter
Unity->>Unity: aggregate sample counts, sort by duration, format times
Unity-->>Server: SuccessResponse { frames, samples }
else not enabled
Unity-->>Server: ErrorResponse ("Profiler not enabled")
end
deactivate Unity
sequenceDiagram
participant Server as Server (Python)
participant Unity as Unity Editor (ReserializeAssets)
participant AssetDB as AssetDatabase (Unity API)
Server->>Unity: send command { path? or paths? }
activate Unity
Unity->>Unity: validate & normalize parameters (prefer `paths` array)
alt paths provided
Unity->>AssetDB: ForceReserializeAssets(paths)
AssetDB-->>Unity: completed
Unity-->>Server: SuccessResponse { count, paths }
else no paths
Unity-->>Server: ErrorResponse ("No paths specified")
end
deactivate Unity
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 4 issues, and left some high level feedback:
- In the Python
manage_profilertool, consider validating and range-checkingframe_count,thread, andmin_msbefore casting (e.g., non-negative, reasonable bounds) to avoid surfacing generic 'Python error' messages for bad user input. - For
ReserializeAssets.HandleCommand, you currently only acceptpathswhen the token is an array; if there's a chance callers might pass a single string forpaths, you may want to normalize non-array values into a one-element list for more robust handling.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the Python `manage_profiler` tool, consider validating and range-checking `frame_count`, `thread`, and `min_ms` before casting (e.g., non-negative, reasonable bounds) to avoid surfacing generic 'Python error' messages for bad user input.
- For `ReserializeAssets.HandleCommand`, you currently only accept `paths` when the token is an array; if there's a chance callers might pass a single string for `paths`, you may want to normalize non-array values into a one-element list for more robust handling.
## Individual Comments
### Comment 1
<location path="MCPForUnity/Editor/Tools/ManageProfiler.cs" line_range="84-93" />
<code_context>
+ continue;
+ }
+
+ var collected = new List<(string name, float ms, int childCount)>();
+ for (int i = 0; i < frameData.sampleCount; i++)
+ {
+ float timeMs = frameData.GetSampleTimeMs(i);
+ if (timeMs < minMs)
+ {
+ continue;
+ }
+
+ string name = frameData.GetSampleName(i);
+ if (string.IsNullOrEmpty(filter) == false &&
+ name.IndexOf(filter, System.StringComparison.OrdinalIgnoreCase) < 0)
+ {
+ continue;
+ }
+
+ collected.Add((name, timeMs, frameData.GetSampleChildrenCount(i)));
+ }
+
</code_context>
<issue_to_address>
**suggestion (performance):** Potentially large sample collections could benefit from a cap or top-N selection.
On frames with many samples and a low `minMs`, `collected` can become very large and then be fully serialized and returned. Consider limiting samples per frame (e.g., keep only the top N by `ms`) or adding a `maxSamples` option to avoid excessively large responses when profiling heavy frames.
Suggested implementation:
```csharp
continue;
}
// Collect samples that pass filters, then cap to the top-N by time to avoid excessively large responses.
var collected = new List<(string name, float ms, int childCount)>();
for (int i = 0; i < frameData.sampleCount; i++)
{
float timeMs = frameData.GetSampleTimeMs(i);
if (timeMs < minMs)
{
continue;
}
string name = frameData.GetSampleName(i);
if (string.IsNullOrEmpty(filter) == false &&
name.IndexOf(filter, System.StringComparison.OrdinalIgnoreCase) < 0)
{
continue;
}
collected.Add((name, timeMs, frameData.GetSampleChildrenCount(i)));
}
// Limit the number of samples per frame to avoid huge payloads on heavy frames.
const int maxSamplesPerFrame = 200;
if (collected.Count > maxSamplesPerFrame)
{
collected = collected
.OrderByDescending(s => s.ms)
.Take(maxSamplesPerFrame)
.ToList();
}
```
1. Ensure `using System.Linq;` is present at the top of `ManageProfiler.cs` so that `OrderByDescending`, `Take`, and `ToList` compile.
2. If you want this limit to be configurable rather than hard-coded, replace the `const int maxSamplesPerFrame = 200;` with a parameter or configuration value (e.g., a method argument or a serialized setting) and thread that value through where this method is called.
</issue_to_address>
### Comment 2
<location path="MCPForUnity/Editor/Tools/ReserializeAssets.cs" line_range="38-39" />
<code_context>
+ {
+ foreach (var item in pathsToken)
+ {
+ string val = item.ToString();
+ if (!string.IsNullOrEmpty(val))
+ {
+ paths.Add(val);
</code_context>
<issue_to_address>
**issue:** Whitespace-only asset paths slip through the current validation.
Because `!string.IsNullOrEmpty(val)` doesn’t filter out whitespace-only strings (e.g., " "), those values will still be added and passed to `ForceReserializeAssets`. Consider using `string.IsNullOrWhiteSpace(val)` and trimming before adding to `paths` to avoid malformed paths.
</issue_to_address>
### Comment 3
<location path="Server/src/services/tools/manage_profiler.py" line_range="58-66" />
<code_context>
+ params: dict[str, Any] = {"action": action}
+
+ if action == "read_frames":
+ if frame_count is not None:
+ params["frameCount"] = int(frame_count)
+ if thread is not None:
+ params["thread"] = int(thread)
+ if filter is not None:
+ params["filter"] = filter
+ if min_ms is not None:
+ params["minMs"] = float(min_ms)
+
+ response = await send_with_unity_instance(
</code_context>
<issue_to_address>
**suggestion:** Direct `int`/`float` casts give a generic error message for invalid numeric inputs.
If `frame_count`, `thread`, or `min_ms` are non-numeric strings, the `int(...)`/`float(...)` calls will raise `ValueError` and be caught by the broad handler, resulting in only the generic "Python error managing profiler" message. Consider catching `ValueError` around these casts and returning a field-specific error (e.g., "frame_count must be an integer"), while letting other exceptions fall through to the generic handler.
```suggestion
if action == "read_frames":
if frame_count is not None:
try:
params["frameCount"] = int(frame_count)
except (TypeError, ValueError):
return {
"success": False,
"message": "frame_count must be an integer.",
}
if thread is not None:
try:
params["thread"] = int(thread)
except (TypeError, ValueError):
return {
"success": False,
"message": "thread must be an integer.",
}
if filter is not None:
params["filter"] = filter
if min_ms is not None:
try:
params["minMs"] = float(min_ms)
except (TypeError, ValueError):
return {
"success": False,
"message": "min_ms must be a number.",
}
```
</issue_to_address>
### Comment 4
<location path="Server/src/services/tools/reserialize_assets.py" line_range="27-29" />
<code_context>
+ str,
+ "Single asset path to reserialize (e.g., 'Assets/Prefabs/Player.prefab')."
+ ] | None = None,
+ paths: Annotated[
+ list[str],
+ "Array of asset paths to reserialize."
+ ] | None = None,
+) -> dict[str, Any]:
</code_context>
<issue_to_address>
**suggestion:** Empty `paths` lists and precedence over `path` could be handled more explicitly.
If both `path` and `paths` are provided, `paths` silently take precedence and `path` is ignored, and an empty `paths` list is forwarded to Unity, which then returns "'path' or 'paths' parameter required". Consider validating that `paths` is non-empty before sending the command and raising a clear client-side error when both `path` and `paths` are set, instead of silently preferring one.
</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
+84
to
+93
| var collected = new List<(string name, float ms, int childCount)>(); | ||
| for (int i = 0; i < frameData.sampleCount; i++) | ||
| { | ||
| float timeMs = frameData.GetSampleTimeMs(i); | ||
| if (timeMs < minMs) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| string name = frameData.GetSampleName(i); |
Contributor
There was a problem hiding this comment.
suggestion (performance): Potentially large sample collections could benefit from a cap or top-N selection.
On frames with many samples and a low minMs, collected can become very large and then be fully serialized and returned. Consider limiting samples per frame (e.g., keep only the top N by ms) or adding a maxSamples option to avoid excessively large responses when profiling heavy frames.
Suggested implementation:
continue;
}
// Collect samples that pass filters, then cap to the top-N by time to avoid excessively large responses.
var collected = new List<(string name, float ms, int childCount)>();
for (int i = 0; i < frameData.sampleCount; i++)
{
float timeMs = frameData.GetSampleTimeMs(i);
if (timeMs < minMs)
{
continue;
}
string name = frameData.GetSampleName(i);
if (string.IsNullOrEmpty(filter) == false &&
name.IndexOf(filter, System.StringComparison.OrdinalIgnoreCase) < 0)
{
continue;
}
collected.Add((name, timeMs, frameData.GetSampleChildrenCount(i)));
}
// Limit the number of samples per frame to avoid huge payloads on heavy frames.
const int maxSamplesPerFrame = 200;
if (collected.Count > maxSamplesPerFrame)
{
collected = collected
.OrderByDescending(s => s.ms)
.Take(maxSamplesPerFrame)
.ToList();
}- Ensure
using System.Linq;is present at the top ofManageProfiler.csso thatOrderByDescending,Take, andToListcompile. - If you want this limit to be configurable rather than hard-coded, replace the
const int maxSamplesPerFrame = 200;with a parameter or configuration value (e.g., a method argument or a serialized setting) and thread that value through where this method is called.
Comment on lines
+38
to
+39
| string val = item.ToString(); | ||
| if (!string.IsNullOrEmpty(val)) |
Contributor
There was a problem hiding this comment.
issue: Whitespace-only asset paths slip through the current validation.
Because !string.IsNullOrEmpty(val) doesn’t filter out whitespace-only strings (e.g., " "), those values will still be added and passed to ForceReserializeAssets. Consider using string.IsNullOrWhiteSpace(val) and trimming before adding to paths to avoid malformed paths.
Comment on lines
+58
to
+66
| if action == "read_frames": | ||
| if frame_count is not None: | ||
| params["frameCount"] = int(frame_count) | ||
| if thread is not None: | ||
| params["thread"] = int(thread) | ||
| if filter is not None: | ||
| params["filter"] = filter | ||
| if min_ms is not None: | ||
| params["minMs"] = float(min_ms) |
Contributor
There was a problem hiding this comment.
suggestion: Direct int/float casts give a generic error message for invalid numeric inputs.
If frame_count, thread, or min_ms are non-numeric strings, the int(...)/float(...) calls will raise ValueError and be caught by the broad handler, resulting in only the generic "Python error managing profiler" message. Consider catching ValueError around these casts and returning a field-specific error (e.g., "frame_count must be an integer"), while letting other exceptions fall through to the generic handler.
Suggested change
| if action == "read_frames": | |
| if frame_count is not None: | |
| params["frameCount"] = int(frame_count) | |
| if thread is not None: | |
| params["thread"] = int(thread) | |
| if filter is not None: | |
| params["filter"] = filter | |
| if min_ms is not None: | |
| params["minMs"] = float(min_ms) | |
| if action == "read_frames": | |
| if frame_count is not None: | |
| try: | |
| params["frameCount"] = int(frame_count) | |
| except (TypeError, ValueError): | |
| return { | |
| "success": False, | |
| "message": "frame_count must be an integer.", | |
| } | |
| if thread is not None: | |
| try: | |
| params["thread"] = int(thread) | |
| except (TypeError, ValueError): | |
| return { | |
| "success": False, | |
| "message": "thread must be an integer.", | |
| } | |
| if filter is not None: | |
| params["filter"] = filter | |
| if min_ms is not None: | |
| try: | |
| params["minMs"] = float(min_ms) | |
| except (TypeError, ValueError): | |
| return { | |
| "success": False, | |
| "message": "min_ms must be a number.", | |
| } |
Comment on lines
+27
to
+29
| paths: Annotated[ | ||
| list[str], | ||
| "Array of asset paths to reserialize." |
Contributor
There was a problem hiding this comment.
suggestion: Empty paths lists and precedence over path could be handled more explicitly.
If both path and paths are provided, paths silently take precedence and path is ignored, and an empty paths list is forwarded to Unity, which then returns "'path' or 'paths' parameter required". Consider validating that paths is non-empty before sending the command and raising a clear client-side error when both path and paths are set, instead of silently preferring one.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
MCPForUnity/Editor/Tools/ReserializeAssets.cs (1)
29-44: Direct@paramsaccess for"paths"bypassesToolParams.
p.Get("path")correctly goes throughToolParams, but@params["paths"]is accessed directly. SinceToolParamshas no array getter, this is constrained by the current API. Consider adding aGetArray/GetStringArrayhelper toToolParamsso both parameters follow the same validated path.As per coding guidelines: "Use
ToolParamsfor consistent parameter validation in C# Editor tools with methods likeGetInt(),RequireString()"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/ReserializeAssets.cs` around lines 29 - 44, The code directly accesses `@params`["paths"] (pathsToken) instead of using the ToolParams helper like p.Get("path"); update ToolParams to add a GetStringArray or GetArray method (e.g., GetStringArray(string key) returning IEnumerable<string> or List<string>) and replace the direct access with p.GetStringArray("paths") (or p.GetArray("paths") then enumerate), preserving existing validation/null checks and trimming/empty-string filtering for each element; update any callers to use the new ToolParams method and keep variable names like pathsToken/paths handling logic consistent with p.Get("path") usage.Server/src/services/tools/manage_profiler.py (2)
44-47:filtershadows the Python built-in.Naming the parameter
filtermasksbuiltins.filter. While it doesn't cause a bug here, it's a recognised Python anti-pattern. Consider renaming toname_filterorsample_filter, and update the corresponding C# param key accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/services/tools/manage_profiler.py` around lines 44 - 47, The parameter name filter in the function signature should be renamed to avoid shadowing builtins; change it to a clearer name like name_filter (or sample_filter) in the manage_profiler.py function signature and everywhere it’s referenced inside that module (e.g., in functions/methods that accept or pass this parameter) and update the corresponding C# parameter key used when calling this service to the same new name so serialization/deserialization still matches; ensure all callers/tests and any docstrings or type annotations referencing filter are updated to the new identifier.
80-81: BroadExceptioncatch and redundantstr()call.Both Ruff warnings here are valid:
- BLE001: Catching bare
Exceptioncan mask programming errors (e.g.,TypeError,AttributeError). Scope the catch to expected transport/IO exceptions, or at minimum re-raise onKeyboardInterrupt/SystemExitfamily.- RUF010:
str(e)should use the!sconversion flag.♻️ Proposed fix
- except Exception as e: - return {"success": False, "message": f"Python error managing profiler: {str(e)}"} + except Exception as e: # noqa: BLE001 + return {"success": False, "message": f"Python error managing profiler: {e!s}"}If narrowing the exception type is feasible (e.g.,
ConnectionError,TimeoutError,ValueError), that is the preferred fix over thenoqasuppression.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/services/tools/manage_profiler.py` around lines 80 - 81, Narrow the broad except in the profiler management code: replace the bare "except Exception as e" with specific expected exceptions (e.g., ConnectionError, TimeoutError, OSError, ValueError) or use "except (ConnectionError, TimeoutError, OSError, ValueError) as e" and re-raise for KeyboardInterrupt and SystemExit if needed; also change the f-string to use the !s conversion flag so the message becomes f"Python error managing profiler: {e!s}". Locate and update the except block that returns {"success": False, "message": ...} in manage_profiler.py.Server/src/services/tools/reserialize_assets.py (1)
56-57: Same broadExceptioncatch andstr(e)issues asmanage_profiler.py.Identical BLE001 and RUF010 violations (see the
manage_profiler.pycomment for rationale). Apply the same fix here for consistency.♻️ Proposed fix
- except Exception as e: - return {"success": False, "message": f"Python error reserializing assets: {str(e)}"} + except Exception as e: # noqa: BLE001 + return {"success": False, "message": f"Python error reserializing assets: {e!s}"}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/services/tools/reserialize_assets.py` around lines 56 - 57, The broad except Exception in the reserialize_assets.py function reserialize_assets should be replaced with targeted exception handling and richer diagnostics: catch only expected exceptions (e.g., OSError, ValueError, json.JSONDecodeError) instead of Exception, and when handling them use the module logger.exception(...) or include traceback.format_exc() in the returned message so you don't rely on str(e) alone; update the except block for reserialize_assets to log the full stack trace and return a concise error payload (or re-raise unexpected exceptions) to mirror the fix applied in manage_profiler.py.MCPForUnity/Editor/Tools/ManageProfiler.cs (1)
60-72:firstFrame < 0guard is overly conservative —-1is the documented "no data" sentinel.
ProfilerDriver.firstFrameIndexandlastFrameIndexreturn-1when no data is available. The current guard (< 0) is correct for that sentinel, but also catches any other negative value. More importantly, after the guard passes, line 72 computeslastFrame - firstFrame + 1, which could still overflow to a very large number iflastFrameis unusually large (e.g. long recording sessions), producing aframeCountin the millions beforeMath.Minreduces it. TheMath.Minon line 72 prevents the loop from overrunning, so there's no correctness bug — but consider adding a reasonable upper-bound clamp onframeCountbefore line 72 to avoid huge allocations when the caller accidentally passesframeCount = Int32.MaxValue.♻️ Proposed clamp
+ const int MaxFrames = 300; frameCount = System.Math.Min(frameCount, lastFrame - firstFrame + 1); + frameCount = System.Math.Min(frameCount, MaxFrames);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/ManageProfiler.cs` around lines 60 - 72, The guard using ProfilerDriver.firstFrameIndex/lastFrameIndex is fine but you should clamp the requested frameCount (from p.GetInt / p.GetInt("frameCount", 1)) to a reasonable upper bound before applying frameCount = System.Math.Min(frameCount, lastFrame - firstFrame + 1) to avoid huge allocations when callers pass extremely large values; update the code that reads frameCount to enforce a max (e.g., MAX_FRAME_REQUEST = 10000) then proceed with the existing Math.Min logic referencing ProfilerDriver.firstFrameIndex and ProfilerDriver.lastFrameIndex.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@MCPForUnity/Editor/Tools/ManageProfiler.cs`:
- Around line 60-72: The guard using
ProfilerDriver.firstFrameIndex/lastFrameIndex is fine but you should clamp the
requested frameCount (from p.GetInt / p.GetInt("frameCount", 1)) to a reasonable
upper bound before applying frameCount = System.Math.Min(frameCount, lastFrame -
firstFrame + 1) to avoid huge allocations when callers pass extremely large
values; update the code that reads frameCount to enforce a max (e.g.,
MAX_FRAME_REQUEST = 10000) then proceed with the existing Math.Min logic
referencing ProfilerDriver.firstFrameIndex and ProfilerDriver.lastFrameIndex.
In `@MCPForUnity/Editor/Tools/ReserializeAssets.cs`:
- Around line 29-44: The code directly accesses `@params`["paths"] (pathsToken)
instead of using the ToolParams helper like p.Get("path"); update ToolParams to
add a GetStringArray or GetArray method (e.g., GetStringArray(string key)
returning IEnumerable<string> or List<string>) and replace the direct access
with p.GetStringArray("paths") (or p.GetArray("paths") then enumerate),
preserving existing validation/null checks and trimming/empty-string filtering
for each element; update any callers to use the new ToolParams method and keep
variable names like pathsToken/paths handling logic consistent with
p.Get("path") usage.
In `@Server/src/services/tools/manage_profiler.py`:
- Around line 44-47: The parameter name filter in the function signature should
be renamed to avoid shadowing builtins; change it to a clearer name like
name_filter (or sample_filter) in the manage_profiler.py function signature and
everywhere it’s referenced inside that module (e.g., in functions/methods that
accept or pass this parameter) and update the corresponding C# parameter key
used when calling this service to the same new name so
serialization/deserialization still matches; ensure all callers/tests and any
docstrings or type annotations referencing filter are updated to the new
identifier.
- Around line 80-81: Narrow the broad except in the profiler management code:
replace the bare "except Exception as e" with specific expected exceptions
(e.g., ConnectionError, TimeoutError, OSError, ValueError) or use "except
(ConnectionError, TimeoutError, OSError, ValueError) as e" and re-raise for
KeyboardInterrupt and SystemExit if needed; also change the f-string to use the
!s conversion flag so the message becomes f"Python error managing profiler:
{e!s}". Locate and update the except block that returns {"success": False,
"message": ...} in manage_profiler.py.
In `@Server/src/services/tools/reserialize_assets.py`:
- Around line 56-57: The broad except Exception in the reserialize_assets.py
function reserialize_assets should be replaced with targeted exception handling
and richer diagnostics: catch only expected exceptions (e.g., OSError,
ValueError, json.JSONDecodeError) instead of Exception, and when handling them
use the module logger.exception(...) or include traceback.format_exc() in the
returned message so you don't rely on str(e) alone; update the except block for
reserialize_assets to log the full stack trace and return a concise error
payload (or re-raise unexpected exceptions) to mirror the fix applied in
manage_profiler.py.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
MCPForUnity/Editor/Tools/ManageProfiler.csMCPForUnity/Editor/Tools/ReserializeAssets.csServer/src/services/tools/manage_profiler.pyServer/src/services/tools/reserialize_assets.py
Collaborator
|
Hi can you use run tests tool to run the editor and play mode tests in the repo's test project. Please also add test coverage for the new feature. Thank you for the contribution, this looks cool |
Python integration tests (14 tests): - manage_profiler: parameter mapping, string coercion, all 5 actions, error passthrough, non-read_frames params isolation - reserialize_assets: single/multiple paths, path priority, missing params validation, error passthrough C# EditMode tests: - ManageProfiler: null params, missing action, unknown action, case insensitivity, status response, enable/disable, clear, read_frames when disabled - ReserializeAssets: null params, missing path/paths, empty paths array Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Author
|
Hi @dsarno, thanks for the review! I've added test coverage in commit Python integration tests (14 tests):
C# EditMode tests:
All 756 Python tests pass (742 existing + 14 new). The C# EditMode tests are in the TestProject and ready to run. Let me know if you'd like any changes! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReserializeAssetsTests.cs (2)
1-45: Only negative/error paths are tested; the success path is absent.
ReserializeAssetsTestscovers null params, missing params, and empty array — all error cases — but has no test where the tool is invoked with a valid path and expected to returnsuccess: true. Even a smoke-test that passes a non-existent path (expecting a Unity-side error vs. a parameter-validation error) would establish a regression baseline for the success branch. IfAssetDatabase.ForceReserializeAssetsis unsafe to call in EditMode, document the reason as a comment so future maintainers understand the gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReserializeAssetsTests.cs` around lines 1 - 45, Add a positive/success-path test to ReserializeAssetsTests that calls ReserializeAssets.HandleCommand with a valid parameters object and asserts that the returned JObject has success==true; locate the test class ReserializeAssetsTests and add a new Test method (e.g., HandleCommand_ValidPath_ReturnsSuccess) that passes either a harmless valid "path" or "paths" value (or a documented non-existent path if AssetDatabase.ForceReserializeAssets cannot be called in EditMode) and verifies success and absence of an error; if calling ForceReserializeAssets is unsafe, add a short comment in the new test explaining why you passed a non-existent path or skipped actual reserialize to document the limitation for future maintainers.
14-14:ToJOhelper is duplicated verbatim fromManageProfilerTests.cs.Extract to a shared static utility class (e.g.,
TestHelpersin the same namespace) to avoid copy-paste drift.♻️ Suggested extraction
// TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/TestHelpers.cs (new file) using Newtonsoft.Json.Linq; namespace MCPForUnityTests.Editor.Tools { internal static class TestHelpers { public static JObject ToJO(object o) => JObject.FromObject(o); } }Then in both test classes, remove the private
ToJOand callTestHelpers.ToJO(result).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReserializeAssetsTests.cs` at line 14, The private helper ToJO in ReserializeAssetsTests.cs is duplicated from ManageProfilerTests.cs; remove the private ToJO method from both test classes and extract it into a shared internal static utility (e.g., TestHelpers with a public/static JObject ToJO(object o)) inside the same namespace (MCPForUnityTests.Editor.Tools), then update both ReserializeAssetsTests and ManageProfilerTests to call TestHelpers.ToJO(...) instead of their private ToJO to eliminate copy-paste drift.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageProfilerTests.cs (2)
44-53:HandleCommand_ActionCaseInsensitivedoesn't assert that either result actually succeeds.
Assert.AreEqual(success, success)is trivially satisfied even if both variants returnfalse(e.g., if the routing is broken and "STATUS" / "status" are both treated as unknown actions). AssertingIsTrueon at least one — or both — of the outcomes gives meaningful signal.♻️ Proposed fix
Assert.AreEqual((bool)upperJo["success"], (bool)lowerJo["success"]); + Assert.IsTrue((bool)upperJo["success"], "Upper-case STATUS should succeed"); + Assert.IsTrue((bool)lowerJo["success"], "Lower-case status should succeed");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageProfilerTests.cs` around lines 44 - 53, The test HandleCommand_ActionCaseInsensitive currently only compares the two results for equality which can be true even if both fail; update the assertions to ensure at least one (or both) call to ManageProfiler.HandleCommand actually succeeded by asserting that the "success" field on the parsed JObject results (upperJo and lowerJo produced from upper and lower) is true — e.g. add Assert.IsTrue((bool)upperJo["success"]) and/or Assert.IsTrue((bool)lowerJo["success"]) after creating upperJo and lowerJo so the test fails when the action routing does not recognize "STATUS"/"status".
92-102: Consider adding a complementary test forread_frameswhen the profiler IS enabled.The current suite only exercises the disabled-profiler error path. A
HandleCommand_ReadFrames_WhenProfilerEnabled_ReturnsFrameDatatest (enable → read_frames → assertsuccess+data.framespresent) would complete the action's coverage and guard against regressions in the success path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageProfilerTests.cs` around lines 92 - 102, Add a complementary unit test that enables the profiler, calls ManageProfiler.HandleCommand with action "read_frames", and asserts the success path: call ManageProfiler.HandleCommand(new JObject { ["action"] = "enable" }) then var result = ManageProfiler.HandleCommand(new JObject { ["action"] = "read_frames" }); convert result with ToJO(result) and assert jo["success"] is true and that jo["data"]["frames"] exists and is non-empty (or has expected shape); name the test HandleCommand_ReadFrames_WhenProfilerEnabled_ReturnsFrameData and mirror setup/teardown used in other tests to avoid state leakage.Server/tests/integration/test_reserialize_assets.py (1)
17-17: Prefix unusedfake_sendarguments with_to silence Ruff ARG001 warnings.Ruff flags all five stubs. If ARG001 is enforced in CI for test files this will break the linting step.
♻️ Proposed fix across all stubs
- async def fake_send(cmd, params, **kwargs): + async def fake_send(cmd, params, **_): captured["cmd"] = cmd captured["params"] = params ...For stubs where
cmdis also unused (Lines 44, 83):- async def fake_send(cmd, params, **kwargs): + async def fake_send(_cmd, params, **_): captured["params"] = params ...For stubs where all args are unused (Lines 105, 125):
- async def fake_send(cmd, params, **kwargs): + async def fake_send(*_args, **_kwargs): ...Also applies to: 44-44, 83-83, 105-105, 125-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/tests/integration/test_reserialize_assets.py` at line 17, The test stubs (e.g., async def fake_send(...)) have unused parameters causing Ruff ARG001; rename unused parameters by prefixing them with an underscore (e.g., change cmd -> _cmd, params -> _params and kwargs -> **_kwargs) for every fake_send stub (including variants where cmd is unused or where all args are unused) so the linter no longer flags ARG001 while preserving function signatures and call behavior.Server/tests/integration/test_manage_profiler.py (1)
17-17: Prefix unusedfake_sendarguments with_to silence Ruff ARG001 (same pattern astest_reserialize_assets.py).All nine stubs carry ARG001 warnings. Apply the same fix described for the reserialize tests: use
_cmd,_params,**_(or*_args, **_kwargsfor fully-unused stubs) so Ruff is satisfied without changing test semantics.Also applies to: 48-48, 81-81, 110-110, 134-134, 154-154, 184-184, 203-203, 221-221
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/tests/integration/test_manage_profiler.py` at line 17, Several test stubs named fake_send (and the other eight similar async stubs) currently have unused parameters triggering Ruff ARG001; update each stub signature to prefix unused positional names with an underscore (e.g. change async def fake_send(cmd, params, **kwargs): to async def fake_send(_cmd, _params, **_):) or, if all args are unused, use *_args, **_kwargs to fully silence the linter while preserving behavior; apply the same change to each occurrence (the nine fake_send stubs referenced) without altering their bodies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReserializeAssetsTests.cs`:
- Around line 35-44: The test HandleCommand_EmptyPathsArray_ReturnsError
currently only asserts success is false; update it to also assert the error
message contains a meaningful substring so failures are clearer. In the
ReserializeAssets.HandleCommand test, after converting result to jo
(ToJO(result)), add an assertion on jo["error"] (e.g.,
Assert.That(jo["error"].ToString(), Does.Contain("paths") or another expected
message) to mirror the checks in HandleCommand_NoPathOrPaths_ReturnsError and
HandleCommand_NullParams_ReturnsError.
---
Nitpick comments:
In `@Server/tests/integration/test_manage_profiler.py`:
- Line 17: Several test stubs named fake_send (and the other eight similar async
stubs) currently have unused parameters triggering Ruff ARG001; update each stub
signature to prefix unused positional names with an underscore (e.g. change
async def fake_send(cmd, params, **kwargs): to async def fake_send(_cmd,
_params, **_):) or, if all args are unused, use *_args, **_kwargs to fully
silence the linter while preserving behavior; apply the same change to each
occurrence (the nine fake_send stubs referenced) without altering their bodies.
In `@Server/tests/integration/test_reserialize_assets.py`:
- Line 17: The test stubs (e.g., async def fake_send(...)) have unused
parameters causing Ruff ARG001; rename unused parameters by prefixing them with
an underscore (e.g., change cmd -> _cmd, params -> _params and kwargs ->
**_kwargs) for every fake_send stub (including variants where cmd is unused or
where all args are unused) so the linter no longer flags ARG001 while preserving
function signatures and call behavior.
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageProfilerTests.cs`:
- Around line 44-53: The test HandleCommand_ActionCaseInsensitive currently only
compares the two results for equality which can be true even if both fail;
update the assertions to ensure at least one (or both) call to
ManageProfiler.HandleCommand actually succeeded by asserting that the "success"
field on the parsed JObject results (upperJo and lowerJo produced from upper and
lower) is true — e.g. add Assert.IsTrue((bool)upperJo["success"]) and/or
Assert.IsTrue((bool)lowerJo["success"]) after creating upperJo and lowerJo so
the test fails when the action routing does not recognize "STATUS"/"status".
- Around line 92-102: Add a complementary unit test that enables the profiler,
calls ManageProfiler.HandleCommand with action "read_frames", and asserts the
success path: call ManageProfiler.HandleCommand(new JObject { ["action"] =
"enable" }) then var result = ManageProfiler.HandleCommand(new JObject {
["action"] = "read_frames" }); convert result with ToJO(result) and assert
jo["success"] is true and that jo["data"]["frames"] exists and is non-empty (or
has expected shape); name the test
HandleCommand_ReadFrames_WhenProfilerEnabled_ReturnsFrameData and mirror
setup/teardown used in other tests to avoid state leakage.
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReserializeAssetsTests.cs`:
- Around line 1-45: Add a positive/success-path test to ReserializeAssetsTests
that calls ReserializeAssets.HandleCommand with a valid parameters object and
asserts that the returned JObject has success==true; locate the test class
ReserializeAssetsTests and add a new Test method (e.g.,
HandleCommand_ValidPath_ReturnsSuccess) that passes either a harmless valid
"path" or "paths" value (or a documented non-existent path if
AssetDatabase.ForceReserializeAssets cannot be called in EditMode) and verifies
success and absence of an error; if calling ForceReserializeAssets is unsafe,
add a short comment in the new test explaining why you passed a non-existent
path or skipped actual reserialize to document the limitation for future
maintainers.
- Line 14: The private helper ToJO in ReserializeAssetsTests.cs is duplicated
from ManageProfilerTests.cs; remove the private ToJO method from both test
classes and extract it into a shared internal static utility (e.g., TestHelpers
with a public/static JObject ToJO(object o)) inside the same namespace
(MCPForUnityTests.Editor.Tools), then update both ReserializeAssetsTests and
ManageProfilerTests to call TestHelpers.ToJO(...) instead of their private ToJO
to eliminate copy-paste drift.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Server/tests/integration/test_manage_profiler.pyServer/tests/integration/test_reserialize_assets.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageProfilerTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReserializeAssetsTests.cs
Comment on lines
+35
to
+44
| [Test] | ||
| public void HandleCommand_EmptyPathsArray_ReturnsError() | ||
| { | ||
| var result = ReserializeAssets.HandleCommand(new JObject | ||
| { | ||
| ["paths"] = new JArray() | ||
| }); | ||
| var jo = ToJO(result); | ||
| Assert.IsFalse((bool)jo["success"]); | ||
| } |
Contributor
There was a problem hiding this comment.
HandleCommand_EmptyPathsArray_ReturnsError doesn't assert an error message.
Both HandleCommand_NoPathOrPaths_ReturnsError and HandleCommand_NullParams_ReturnsError check the error string, but this test only verifies success == false. Adding a Does.Contain assertion here makes failures easier to diagnose and ensures the tool returns a meaningful message, not just a bare negative flag.
♻️ Proposed fix
Assert.IsFalse((bool)jo["success"]);
+ Assert.IsNotNull(jo["error"]);
+ Assert.That((string)jo["error"], Does.Contain("path").Or.Contain("empty").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.
Suggested change
| [Test] | |
| public void HandleCommand_EmptyPathsArray_ReturnsError() | |
| { | |
| var result = ReserializeAssets.HandleCommand(new JObject | |
| { | |
| ["paths"] = new JArray() | |
| }); | |
| var jo = ToJO(result); | |
| Assert.IsFalse((bool)jo["success"]); | |
| } | |
| [Test] | |
| public void HandleCommand_EmptyPathsArray_ReturnsError() | |
| { | |
| var result = ReserializeAssets.HandleCommand(new JObject | |
| { | |
| ["paths"] = new JArray() | |
| }); | |
| var jo = ToJO(result); | |
| Assert.IsFalse((bool)jo["success"]); | |
| Assert.IsNotNull(jo["error"]); | |
| Assert.That((string)jo["error"], Does.Contain("path").Or.Contain("empty").IgnoreCase); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ReserializeAssetsTests.cs`
around lines 35 - 44, The test HandleCommand_EmptyPathsArray_ReturnsError
currently only asserts success is false; update it to also assert the error
message contains a meaningful substring so failures are clearer. In the
ReserializeAssets.HandleCommand test, after converting result to jo
(ToJO(result)), add an assertion on jo["error"] (e.g.,
Assert.That(jo["error"].ToString(), Does.Contain("paths") or another expected
message) to mirror the checks in HandleCommand_NoPathOrPaths_ReturnsError and
HandleCommand_NullParams_ReturnsError.
Collaborator
|
Thank you, but can you actually run the editor Also, have you tested that when you reserializd, it does not break connections between existing gameobjects,assets and components etc? |
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
2 participants
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.
Summary
manage_profiler: Reads Unity Profiler frame data directly viaProfilerDriver.GetRawFrameDataView, providing the same data visible in the Profiler window. Supports multiple actions:read_frames— Read profiler samples from recent frames with filtering by thread, sample name, and minimum time thresholdenable/disable— Control profiler recording statestatus— Query current profiler state (enabled, frame count, etc.)clear— Clear all recorded framesreserialize_assets: Forces Unity to reserialize assets viaAssetDatabase.ForceReserializeAssets. Accepts a singlepathor an array ofpaths. Useful after Unity version upgrades, script changes affecting serialized data, or when assets need their .meta files regenerated.Both tools follow the existing C# + Python architecture with
[McpForUnityTool]attribute registration and@mcp_for_unity_tooldecorator.Test plan
pytest tests/ -x→ 0 failures)🤖 Generated with Claude Code
Summary by Sourcery
Add new Unity editor and Python tools for managing the Unity Profiler and forcing asset reserialization via the existing MCP for Unity tooling pipeline.
New Features:
Summary by CodeRabbit
New Features
Tests