Update0503#1097
Merged
Scriptwonder merged 2 commits intoMay 4, 2026
Merged
Conversation
1.Add Compat based scripts revolving around UnityCompatShims.cs, that will document our current API Compatibility changes in several files. 2.Add custom screenshot folder selection
Contributor
📝 WalkthroughWalkthroughThis PR centralizes Unity API compatibility shims, replaces Assets-relative screenshot APIs with project-relative folder handling (with per-call and EditorPrefs overrides), updates multiple editor tools to use the new compat and screenshot helpers, and threads an optional output-folder parameter through server CLI and services. ChangesCompatibility, Screenshot Pathing & Output-Folder Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR expands cross-Unity-version support by introducing a documented “compat shim” layer for fragile/obsolete Unity APIs, and adds configurable screenshot output folder selection (per-call + Editor preference) across camera/scene/UI screenshot flows.
Changes:
- Add new Unity compatibility shims (Physics simulation/settings, loaded-assemblies enumeration) and document the shim policy via
UnityCompatShims+CLAUDE.md. - Add screenshot output folder override (
output_folder/outputFolder) end-to-end (Server tools + CLI + Unity Editor tools), plus an EditorPrefs-backed default folder setting in the Advanced UI. - Refactor screenshot utilities/results to use project-relative paths and only import into AssetDatabase when the file is under
Assets/.
Reviewed changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| Server/uv.lock | Bumps server package version. |
| Server/src/services/tools/utils.py | Adds output_folder → outputFolder parameter forwarding for screenshot commands. |
| Server/src/services/tools/manage_ui.py | Adds output_folder parameter to render_ui routing. |
| Server/src/services/tools/manage_camera.py | Adds output_folder parameter and forwards it through shared screenshot param builder. |
| Server/src/cli/commands/camera.py | Adds --output-folder flag to camera screenshot CLI commands. |
| MCPForUnity/Runtime/Helpers/UnityPhysicsCompat.cs | New reflection-based shim for Physics/Physics2D sync + simulation mode across Unity versions. |
| MCPForUnity/Runtime/Helpers/UnityPhysicsCompat.cs.meta | Unity asset metadata for the new shim. |
| MCPForUnity/Runtime/Helpers/UnityAssembliesCompat.cs | New shim for loaded-assembly enumeration (CoreCLR-forward). |
| MCPForUnity/Runtime/Helpers/UnityAssembliesCompat.cs.meta | Unity asset metadata for the new shim. |
| MCPForUnity/Runtime/Helpers/UnityCompatShims.cs | Adds an in-package “catalog + policy” anchor for all compat shims. |
| MCPForUnity/Runtime/Helpers/UnityCompatShims.cs.meta | Unity asset metadata for the shim catalog marker. |
| MCPForUnity/Runtime/Helpers/UnityFindObjectsCompat.cs | Updates gating + uses reflection to avoid obsolete API warnings on legacy Unity versions. |
| MCPForUnity/Runtime/Helpers/UnityObjectIdCompat.cs | Adds shim-family header comment for consistency. |
| MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs | Adds folder override + path semantics change to project-relative; validates folders stay inside project root. |
| MCPForUnity/Editor/Helpers/ScreenshotPreferences.cs | New EditorPrefs-based default screenshot folder resolver (override → pref → built-in default). |
| MCPForUnity/Editor/Helpers/ScreenshotPreferences.cs.meta | Unity asset metadata for the new preference helper. |
| MCPForUnity/Editor/Helpers/EditorWindowScreenshotUtility.cs | Routes SceneView capture into project folder + returns project-relative path. |
| MCPForUnity/Editor/Windows/Components/Advanced/McpAdvancedSection.uxml | Adds Advanced UI controls for screenshot folder preference. |
| MCPForUnity/Editor/Windows/Components/Advanced/McpAdvancedSection.cs | Wires screenshot folder preference controls + browse/clear actions. |
| MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.cs | Updates tooltips + writes multiview captures to the resolved screenshot folder. |
| MCPForUnity/Editor/Tools/ManageScene.cs | Applies folder override to screenshot capture paths; imports only when under Assets/. |
| MCPForUnity/Editor/Tools/ManageUI.cs | Applies folder override to render_ui output + returns project-relative paths. |
| MCPForUnity/Editor/Tools/Physics/PhysicsSimulationOps.cs | Switches to compat shim for simulation mode handling during manual simulate steps. |
| MCPForUnity/Editor/Tools/Physics/PhysicsSettingsOps.cs | Uses compat shim for simulation mode + sync transform access across versions. |
| MCPForUnity/Editor/Tools/UnityReflect.cs | Switches assembly enumeration to UnityAssembliesCompat. |
| MCPForUnity/Editor/Tools/ExecuteCode.cs | Switches assembly enumeration to UnityAssembliesCompat. |
| MCPForUnity/Editor/Tools/CommandRegistry.cs | Switches assembly enumeration to UnityAssembliesCompat. |
| MCPForUnity/Editor/Tools/Animation/ClipCreate.cs | Switches assembly enumeration to UnityAssembliesCompat. |
| MCPForUnity/Editor/Helpers/UnityTypeResolver.cs | Switches assembly enumeration to UnityAssembliesCompat. |
| CLAUDE.md | Documents the “compat shim” approach and points to UnityCompatShims.cs as the source of truth. |
Comments suppressed due to low confidence (1)
MCPForUnity/Editor/Tools/Physics/PhysicsSimulationOps.cs:38
TrySetPhysicsSimulationMode(Script)return value is ignored. If the shim cannot switch to Script mode (e.g. API removed/changed in a future Unity),Physics.Simulatewill throw and the tool will fail unexpectedly. Consider checking the return value and returning anErrorResponsewhen the mode can't be set.
UnityEngine.Physics.SyncTransforms();
var prevMode = UnityPhysicsCompat.GetPhysicsSimulationMode();
if (prevMode != UnityPhysicsCompat.SimulationMode.Script)
UnityPhysicsCompat.TrySetPhysicsSimulationMode(UnityPhysicsCompat.SimulationMode.Script);
try
{
for (int i = 0; i < steps; i++)
UnityEngine.Physics.Simulate(stepSize);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+557
to
+562
| ? string.Empty | ||
| : normalizedPicked.Substring(normalizedRoot.Length); | ||
|
|
||
| ScreenshotPreferences.DefaultFolder = projectRelative; | ||
| screenshotsFolderOverride?.SetValueWithoutNotify(projectRelative); | ||
| McpLog.Info($"Default screenshots folder set to '{(string.IsNullOrEmpty(projectRelative) ? "(project root)" : projectRelative)}'."); |
Comment on lines
918
to
922
| var playData = new Dictionary<string, object> | ||
| { | ||
| { "path", playAssetsRelPath }, | ||
| { "path", playProjectRelPath }, | ||
| { "fullPath", playFullPath }, | ||
| { "width", captureW }, |
Comment on lines
+614
to
+626
| string folderOverride = ScreenshotPreferences.Resolve(cmd.outputFolder); | ||
| ScreenshotCaptureResult result; | ||
| try | ||
| { | ||
| result = ScreenshotUtility.CaptureFromCameraToProjectFolder( | ||
| targetCamera, fileName, resolvedSuperSize, ensureUniqueFileName: true, | ||
| includeImage: includeImage, maxResolution: maxResolution, | ||
| folderOverride: folderOverride); | ||
| } | ||
| catch (InvalidOperationException ex) | ||
| { | ||
| return new ErrorResponse(ex.Message); | ||
| } |
Comment on lines
+485
to
+486
| if output_folder: | ||
| params["outputFolder"] = output_folder.strip() |
| /// <summary> | ||
| /// Captures a single screenshot from a temporary camera placed at view_position and aimed at view_target. | ||
| /// Returns inline base64 PNG and also saves the image to Assets/Screenshots/. | ||
| /// Returns inline base64 PNG and also saves the image to <projectRoot>/Captures/. |
Comment on lines
+1828
to
+1832
| string projectRoot = Path.GetFullPath(Path.Combine(Application.dataPath, "..")).Replace('\\', '/'); | ||
| string normalizedRoot = projectRoot.EndsWith("/") ? projectRoot : projectRoot + "/"; | ||
| return normalizedFullPath.StartsWith(normalizedRoot, StringComparison.OrdinalIgnoreCase) | ||
| ? normalizedFullPath.Substring(normalizedRoot.Length) | ||
| : normalizedFullPath; |
Comment on lines
223
to
+228
| if max_resolution is not None: | ||
| params_dict["max_resolution"] = max_resolution | ||
| if screenshot_file_name is not None: | ||
| params_dict["file_name"] = screenshot_file_name | ||
| if output_folder is not None and output_folder.strip(): | ||
| params_dict["output_folder"] = output_folder.strip() |
Comment on lines
632
to
636
| var data = new Dictionary<string, object> | ||
| { | ||
| { "path", result.AssetsRelativePath }, | ||
| { "path", result.ProjectRelativePath }, | ||
| { "fullPath", result.FullPath }, | ||
| { "superSize", result.SuperSize }, |
![coderabbitai[bot]](https://avatars.gh.lumenfield.work/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Server/src/cli/commands/camera.py (1)
491-525: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd tests for the new
--output-folderoption before merge.This adds two new public CLI parameters, but I don't see matching coverage in this PR context. Please add at least one happy-path case that forwards a project-relative folder and one validation case for an outside-project path.
Based on learnings: Every new feature must include tests before submission.
Also applies to: 533-552
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/cli/commands/camera.py` around lines 491 - 525, Add unit/CLI integration tests for the new --output-folder option in the screenshot command: create one happy-path test that invokes the screenshot CLI (function screenshot) with a project-relative folder string and asserts the outgoing request contains params["outputFolder"] set to that value, and create one validation test that passes an absolute/outside-project path and asserts the command rejects it (error/validation behavior) or does not set params["outputFolder"]; place tests alongside existing camera command tests and reuse the same test helpers/mocks used for other screenshot parameter checks to simulate request payloads and verify behavior.MCPForUnity/Editor/Tools/ManageScene.cs (1)
1174-1191:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReject path-like
fileNamevalues before writing the positioned screenshot.Unlike the other screenshot paths, this branch uses
cmd.fileNamedirectly inPath.Combine(folderAbsolute, fileName). A caller can pass separators or..here and escape the resolved folder — with enough traversal, even the project root.Suggested fix
- string fileName = !string.IsNullOrEmpty(cmd.fileName) - ? (cmd.fileName.EndsWith(".png", System.StringComparison.OrdinalIgnoreCase) ? cmd.fileName : cmd.fileName + ".png") - : $"screenshot-{DateTime.Now:yyyyMMdd-HHmmss}.png"; + string requestedFileName = string.IsNullOrWhiteSpace(cmd.fileName) + ? $"screenshot-{DateTime.Now:yyyyMMdd-HHmmss}.png" + : Path.GetFileName(cmd.fileName.Trim()); + if (string.IsNullOrEmpty(requestedFileName) || requestedFileName == "." || requestedFileName == "..") + return new ErrorResponse("fileName must be a file name, not a path."); + string fileName = requestedFileName.EndsWith(".png", StringComparison.OrdinalIgnoreCase) + ? requestedFileName + : requestedFileName + ".png";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/ManageScene.cs` around lines 1174 - 1191, Reject or sanitize any path-like values in cmd.fileName before combining and writing: treat cmd.fileName only as a basename (use Path.GetFileName or equivalent to strip directory separators and ..), enforce/append the .png extension as currently done, then build fullPath via Path.Combine(folderAbsolute, sanitizedFileName) and verify the resulting Path.GetFullPath(fullPath) starts with Path.GetFullPath(folderAbsolute) to prevent directory traversal before calling File.WriteAllBytes; update the logic around fileName, fullPath, and the existing existence/uniqueness loop to operate on the sanitized basename.MCPForUnity/Runtime/Helpers/UnityFindObjectsCompat.cs (1)
61-67:⚠️ Potential issue | 🟠 MajorChange the gate from
UNITY_2023_1_OR_NEWERtoUNITY_2022_3_OR_NEWER.The 3-parameter overload
FindObjectsByType(Type, FindObjectsInactive, FindObjectsSortMode)is available in Unity 2022.3 LTS, as documented in the official API reference. The current gate atUNITY_2023_1_OR_NEWERunnecessarily restricts 2022.3 to the reflection fallback. This also creates an inconsistency—theFindAll(Type)method at line 44 correctly gates the same API family atUNITY_2022_3_OR_NEWER. The file's own API timeline comment (lines 12–15) confirmsFindObjectsByType(sortMode)is available in "2022.3-6.4". Change the gate to align with the actual API availability and match the pattern used elsewhere in this file.Proposed fix
-#elif UNITY_2023_1_OR_NEWER +#elif UNITY_2022_3_OR_NEWER return UObject.FindObjectsByType(type, includeInactive ? UnityEngine.FindObjectsInactive.Include : UnityEngine.FindObjectsInactive.Exclude, UnityEngine.FindObjectsSortMode.None);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Runtime/Helpers/UnityFindObjectsCompat.cs` around lines 61 - 67, Update the preprocessor gate for the FindObjectsByType call: replace the UNITY_2023_1_OR_NEWER conditional around the return that invokes UObject.FindObjectsByType(type, includeInactive ? UnityEngine.FindObjectsInactive.Include : UnityEngine.FindObjectsInactive.Exclude, UnityEngine.FindObjectsSortMode.None) with UNITY_2022_3_OR_NEWER so the three-parameter overload is used for Unity 2022.3 LTS (matching the existing gate used for FindAll(Type) and the file's API timeline) instead of falling back to LegacyFindObjectsOfType.
🧹 Nitpick comments (1)
MCPForUnity/Editor/Tools/Physics/PhysicsSimulationOps.cs (1)
32-46: ⚡ Quick winConsider checking the return value of
TrySetPhysicsSimulationModebefore simulating.If
TrySetPhysicsSimulationMode(Script)fails on line 34, the simulation loop proceeds anyway, andPhysics.Simulate()will throw an unclear Unity exception because the mode wasn't set toScript. Checking the return value and returning anErrorResponsewould provide a clearer failure message.♻️ Proposed refactor
var prevMode = UnityPhysicsCompat.GetPhysicsSimulationMode(); if (prevMode != UnityPhysicsCompat.SimulationMode.Script) - UnityPhysicsCompat.TrySetPhysicsSimulationMode(UnityPhysicsCompat.SimulationMode.Script); +{ + if (!UnityPhysicsCompat.TrySetPhysicsSimulationMode(UnityPhysicsCompat.SimulationMode.Script)) + return new ErrorResponse("Cannot set physics simulation mode to Script. Physics.Simulate() requires Script mode."); +} try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/Physics/PhysicsSimulationOps.cs` around lines 32 - 46, Before running the simulation loop in PhysicsSimulationOps, check the boolean return of UnityPhysicsCompat.TrySetPhysicsSimulationMode(UnityPhysicsCompat.SimulationMode.Script) and abort with an ErrorResponse if it returns false; do not call UnityEngine.Physics.Simulate when setting the mode failed. Keep the existing finally block that restores the previous mode via UnityPhysicsCompat.TrySetPhysicsSimulationMode(prevMode) (only when prevMode != Unknown and != Script) so the restore still runs on early exit.
🤖 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/ScreenshotPreferences.cs`:
- Around line 24-33: In the setter that currently trims and writes to
EditorPrefs (using EditorPrefsKey and EditorPrefs.SetString/DeleteKey), validate
the incoming value before persisting: trim the value, treat empty/whitespace the
same (call EditorPrefs.DeleteKey), otherwise resolve the path and reject or
convert invalid absolute paths—if Path.IsPathRooted then check if the full path
is inside the Unity project (compare against Application.dataPath) and if so
convert to a project-relative path; if it points outside the project, do not
call EditorPrefs.SetString and surface a warning (e.g., Debug.LogWarning or an
Editor dialog); for relative paths ensure the directory exists (create with
Directory.CreateDirectory if needed) before calling
EditorPrefs.SetString(EditorPrefsKey, trimmedValue).
In `@MCPForUnity/Editor/Tools/ManageScene.cs`:
- Around line 1232-1247: ResolveAbsoluteOutputFolder currently swallows all
exceptions and returns ScreenshotUtility.DefaultFolder, which hides invalid
caller paths; instead stop silently replacing the caller value: call
ScreenshotPreferences.Resolve(callerOverride) and pass the returned spec into
ScreenshotUtility.ResolveFolderAbsolute(spec) without a catch that falls back to
DefaultFolder, or catch only to wrap and rethrow a descriptive exception
(including the spec) so callers see the validation error; update
ResolveAbsoluteOutputFolder to either let ResolveFolderAbsolute throw or to
rethrow a new exception with context (reference: ResolveAbsoluteOutputFolder,
ScreenshotPreferences.Resolve, ScreenshotUtility.ResolveFolderAbsolute,
ScreenshotUtility.DefaultFolder).
In `@MCPForUnity/Editor/Tools/ManageUI.cs`:
- Around line 863-879: Add unit tests covering the new screenshot-folder flow:
test ScreenshotPreferences.Resolve fallback behavior (when output_folder vs
outputFolder), validate ScreenshotUtility.ResolveFolderAbsolute throws and that
the calling method returns an ErrorResponse with the exception message, exercise
the project-relative path reporting branch (paths inside and outside "Assets/"),
and verify that AssetDatabase.ImportAsset is only invoked for paths under
"Assets/". Target the code paths exercised by ManageUI methods that call
ScreenshotPreferences.Resolve, ScreenshotUtility.ResolveFolderAbsolute and
construct ErrorResponse so the preference fallback, invalid-folder rejection,
project-relative reporting, and conditional ImportAsset branches are each
asserted.
In `@MCPForUnity/Editor/Windows/Components/Advanced/McpAdvancedSection.cs`:
- Around line 556-562: The code treats an empty projectRelative as "unset" which
loses a user selecting the project root; instead detect the project-root case
(when normalizedPicked.Equals(projectRoot)) and set a clear sentinel (e.g. "."
or another agreed token) rather than string.Empty, then assign that sentinel to
ScreenshotPreferences.DefaultFolder and pass it to
screenshotsFolderOverride.SetValueWithoutNotify; update the McpLog.Info message
to check for the sentinel and print "(project root)" when present while
otherwise printing the actual relative path (use the symbols normalizedPicked,
projectRoot, projectRelative, ScreenshotPreferences.DefaultFolder,
screenshotsFolderOverride, and McpLog.Info to locate and change the logic).
In `@MCPForUnity/Runtime/Helpers/ScreenshotUtility.cs`:
- Around line 561-578: PrepareCaptureResult currently allows backslashes from
the caller's fileName to survive BuildFileName and then get turned into '/'
after fullPath.Replace('\\','/'), which can create unintended nested paths; fix
by sanitizing/removing backslash characters from the incoming fileName before
calling BuildFileName (or immediately after BuildFileName) so that
BuildFileName/EnsureUnique operate on a filename without '\' characters, then
continue with ResolveFolderAbsolute, Directory.CreateDirectory, Path.Combine,
Replace('\\','/'), ToProjectRelativePath and return the ScreenshotCaptureResult
as before.
- Around line 598-605: The containment check for fullFolder against
normalizedRoot uses StringComparison.OrdinalIgnoreCase which is incorrect on
case-sensitive filesystems; change the comparison to use a platform-appropriate
StringComparison (use OrdinalIgnoreCase on Windows, Ordinal on Linux/macOS) and
apply that single variable to both the Equals and StartsWith checks (references:
fullFolder, normalizedRoot, Equals, StartsWith,
StringComparison.OrdinalIgnoreCase, folderOverride). Ensure you compute the
comparison mode once (e.g., comparisonMode) using RuntimeInformation/Platform
detection and use it in the two existing path comparisons so the rejection logic
behaves correctly across platforms.
In `@MCPForUnity/Runtime/Helpers/UnityAssembliesCompat.cs`:
- Around line 22-120: Add editor tests that pin the new discovery/compat paths
so upgrades don't silently break auto-registration or object lookup: create
tests that exercise UnityAssembliesCompat.GetLoadedAssemblies and
ResolveCurrentAssembliesDelegate (verify CurrentAssembliesAqns entry resolution
and the fallback scan path both return a sensible set including a known test
assembly/type) and tests that exercise UnityFindObjectsCompat's version-routing
branches (simulate both 2022.3 and 6000.x behaviors and assert
UnityTypeResolver/CommandRegistry auto-registration finds a known test type and
that FindObjects behavior returns expected objects); if Type.GetType/AQN
resolution is unavailable in the test runner, force the fallback by preloading a
test assembly and assert the delegate returned from TryBindGetLoadedAssemblies
behaves the same as AppDomain.CurrentDomain.GetAssemblies.
---
Outside diff comments:
In `@MCPForUnity/Editor/Tools/ManageScene.cs`:
- Around line 1174-1191: Reject or sanitize any path-like values in cmd.fileName
before combining and writing: treat cmd.fileName only as a basename (use
Path.GetFileName or equivalent to strip directory separators and ..),
enforce/append the .png extension as currently done, then build fullPath via
Path.Combine(folderAbsolute, sanitizedFileName) and verify the resulting
Path.GetFullPath(fullPath) starts with Path.GetFullPath(folderAbsolute) to
prevent directory traversal before calling File.WriteAllBytes; update the logic
around fileName, fullPath, and the existing existence/uniqueness loop to operate
on the sanitized basename.
In `@MCPForUnity/Runtime/Helpers/UnityFindObjectsCompat.cs`:
- Around line 61-67: Update the preprocessor gate for the FindObjectsByType
call: replace the UNITY_2023_1_OR_NEWER conditional around the return that
invokes UObject.FindObjectsByType(type, includeInactive ?
UnityEngine.FindObjectsInactive.Include :
UnityEngine.FindObjectsInactive.Exclude, UnityEngine.FindObjectsSortMode.None)
with UNITY_2022_3_OR_NEWER so the three-parameter overload is used for Unity
2022.3 LTS (matching the existing gate used for FindAll(Type) and the file's API
timeline) instead of falling back to LegacyFindObjectsOfType.
In `@Server/src/cli/commands/camera.py`:
- Around line 491-525: Add unit/CLI integration tests for the new
--output-folder option in the screenshot command: create one happy-path test
that invokes the screenshot CLI (function screenshot) with a project-relative
folder string and asserts the outgoing request contains params["outputFolder"]
set to that value, and create one validation test that passes an
absolute/outside-project path and asserts the command rejects it
(error/validation behavior) or does not set params["outputFolder"]; place tests
alongside existing camera command tests and reuse the same test helpers/mocks
used for other screenshot parameter checks to simulate request payloads and
verify behavior.
---
Nitpick comments:
In `@MCPForUnity/Editor/Tools/Physics/PhysicsSimulationOps.cs`:
- Around line 32-46: Before running the simulation loop in PhysicsSimulationOps,
check the boolean return of
UnityPhysicsCompat.TrySetPhysicsSimulationMode(UnityPhysicsCompat.SimulationMode.Script)
and abort with an ErrorResponse if it returns false; do not call
UnityEngine.Physics.Simulate when setting the mode failed. Keep the existing
finally block that restores the previous mode via
UnityPhysicsCompat.TrySetPhysicsSimulationMode(prevMode) (only when prevMode !=
Unknown and != Script) so the restore still runs on early exit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c1134546-c58d-4944-8b79-2a6ecf47510d
⛔ Files ignored due to path filters (1)
Server/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
CLAUDE.mdMCPForUnity/Editor/Helpers/EditorWindowScreenshotUtility.csMCPForUnity/Editor/Helpers/ScreenshotPreferences.csMCPForUnity/Editor/Helpers/ScreenshotPreferences.cs.metaMCPForUnity/Editor/Helpers/UnityTypeResolver.csMCPForUnity/Editor/Tools/Animation/ClipCreate.csMCPForUnity/Editor/Tools/CommandRegistry.csMCPForUnity/Editor/Tools/ExecuteCode.csMCPForUnity/Editor/Tools/ManageScene.csMCPForUnity/Editor/Tools/ManageUI.csMCPForUnity/Editor/Tools/Physics/PhysicsSettingsOps.csMCPForUnity/Editor/Tools/Physics/PhysicsSimulationOps.csMCPForUnity/Editor/Tools/UnityReflect.csMCPForUnity/Editor/Windows/Components/Advanced/McpAdvancedSection.csMCPForUnity/Editor/Windows/Components/Advanced/McpAdvancedSection.uxmlMCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.csMCPForUnity/Runtime/Helpers/ScreenshotUtility.csMCPForUnity/Runtime/Helpers/UnityAssembliesCompat.csMCPForUnity/Runtime/Helpers/UnityAssembliesCompat.cs.metaMCPForUnity/Runtime/Helpers/UnityCompatShims.csMCPForUnity/Runtime/Helpers/UnityCompatShims.cs.metaMCPForUnity/Runtime/Helpers/UnityFindObjectsCompat.csMCPForUnity/Runtime/Helpers/UnityObjectIdCompat.csMCPForUnity/Runtime/Helpers/UnityPhysicsCompat.csMCPForUnity/Runtime/Helpers/UnityPhysicsCompat.cs.metaServer/src/cli/commands/camera.pyServer/src/services/tools/manage_camera.pyServer/src/services/tools/manage_ui.pyServer/src/services/tools/utils.py
Comment on lines
+24
to
+33
| set | ||
| { | ||
| if (string.IsNullOrWhiteSpace(value)) | ||
| { | ||
| EditorPrefs.DeleteKey(EditorPrefsKey); | ||
| } | ||
| else | ||
| { | ||
| EditorPrefs.SetString(EditorPrefsKey, value.Trim()); | ||
| } |
Contributor
There was a problem hiding this comment.
Validate the folder override before persisting it.
This setter currently stores any nonblank string. Because the new Advanced Settings field is editable, a typo or out-of-project absolute path becomes a sticky EditorPrefs value that makes later screenshot calls fail until the user clears it.
Suggested fix
public static string DefaultFolder
{
get => EditorPrefs.GetString(EditorPrefsKey, string.Empty);
set
{
if (string.IsNullOrWhiteSpace(value))
{
EditorPrefs.DeleteKey(EditorPrefsKey);
}
else
{
- EditorPrefs.SetString(EditorPrefsKey, value.Trim());
+ string trimmed = value.Trim();
+ ScreenshotUtility.ResolveFolderAbsolute(trimmed);
+ EditorPrefs.SetString(EditorPrefsKey, trimmed);
}
}
}📝 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
| set | |
| { | |
| if (string.IsNullOrWhiteSpace(value)) | |
| { | |
| EditorPrefs.DeleteKey(EditorPrefsKey); | |
| } | |
| else | |
| { | |
| EditorPrefs.SetString(EditorPrefsKey, value.Trim()); | |
| } | |
| set | |
| { | |
| if (string.IsNullOrWhiteSpace(value)) | |
| { | |
| EditorPrefs.DeleteKey(EditorPrefsKey); | |
| } | |
| else | |
| { | |
| string trimmed = value.Trim(); | |
| ScreenshotUtility.ResolveFolderAbsolute(trimmed); | |
| EditorPrefs.SetString(EditorPrefsKey, trimmed); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Helpers/ScreenshotPreferences.cs` around lines 24 - 33, In
the setter that currently trims and writes to EditorPrefs (using EditorPrefsKey
and EditorPrefs.SetString/DeleteKey), validate the incoming value before
persisting: trim the value, treat empty/whitespace the same (call
EditorPrefs.DeleteKey), otherwise resolve the path and reject or convert invalid
absolute paths—if Path.IsPathRooted then check if the full path is inside the
Unity project (compare against Application.dataPath) and if so convert to a
project-relative path; if it points outside the project, do not call
EditorPrefs.SetString and surface a warning (e.g., Debug.LogWarning or an Editor
dialog); for relative paths ensure the directory exists (create with
Directory.CreateDirectory if needed) before calling
EditorPrefs.SetString(EditorPrefsKey, trimmedValue).
Comment on lines
+863
to
+879
| string outputFolderOverride = p.Get("output_folder") ?? p.Get("outputFolder"); | ||
|
|
||
| if (string.IsNullOrEmpty(target) && string.IsNullOrEmpty(uxmlPath)) | ||
| { | ||
| return new ErrorResponse("Either 'target' (GameObject with UIDocument) or 'path' (UXML asset path) is required."); | ||
| } | ||
|
|
||
| string resolvedFolderSpec = ScreenshotPreferences.Resolve(outputFolderOverride); | ||
| string resolvedFolderAbs; | ||
| try | ||
| { | ||
| resolvedFolderAbs = ScreenshotUtility.ResolveFolderAbsolute(resolvedFolderSpec); | ||
| } | ||
| catch (InvalidOperationException ex) | ||
| { | ||
| return new ErrorResponse(ex.Message); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Please add coverage for the new screenshot-folder flow before merge.
This change adds several regression-prone branches—preference fallback, invalid-folder rejection, project-relative path reporting, and AssetDatabase.ImportAsset only under Assets/—but I don't see accompanying tests in the provided changes.
Based on learnings: "Every new feature must include tests before submission."
Also applies to: 897-917, 1149-1158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Tools/ManageUI.cs` around lines 863 - 879, Add unit tests
covering the new screenshot-folder flow: test ScreenshotPreferences.Resolve
fallback behavior (when output_folder vs outputFolder), validate
ScreenshotUtility.ResolveFolderAbsolute throws and that the calling method
returns an ErrorResponse with the exception message, exercise the
project-relative path reporting branch (paths inside and outside "Assets/"), and
verify that AssetDatabase.ImportAsset is only invoked for paths under "Assets/".
Target the code paths exercised by ManageUI methods that call
ScreenshotPreferences.Resolve, ScreenshotUtility.ResolveFolderAbsolute and
construct ErrorResponse so the preference fallback, invalid-folder rejection,
project-relative reporting, and conditional ImportAsset branches are each
asserted.
Comment on lines
+22
to
+120
| public static class UnityAssembliesCompat | ||
| { | ||
| // Candidate assembly-qualified names to try BEFORE falling back to a full | ||
| // assembly scan. Probing by name avoids calling AppDomain.GetAssemblies on | ||
| // CoreCLR (where it emits warnings) in the common case. | ||
| private static readonly string[] CurrentAssembliesAqns = | ||
| { | ||
| "UnityEngine.Assemblies.CurrentAssemblies, UnityEngine.CoreModule", | ||
| "UnityEngine.Assemblies.CurrentAssemblies, UnityEngine", | ||
| "UnityEngine.Assemblies.CurrentAssemblies, UnityEditor.CoreModule", | ||
| "UnityEngine.Assemblies.CurrentAssemblies, UnityEditor", | ||
| }; | ||
|
|
||
| private static Func<Assembly[]> _getLoadedAssemblies; | ||
| private static bool _probed; | ||
|
|
||
| /// <summary> | ||
| /// Returns all currently loaded managed assemblies in this Unity process. | ||
| /// On Unity 6.8+ (CoreCLR) this dispatches to | ||
| /// <c>UnityEngine.Assemblies.CurrentAssemblies.GetLoadedAssemblies()</c>; | ||
| /// otherwise falls back to <c>AppDomain.CurrentDomain.GetAssemblies()</c>. | ||
| /// </summary> | ||
| public static Assembly[] GetLoadedAssemblies() | ||
| { | ||
| if (!_probed) | ||
| { | ||
| _probed = true; | ||
| _getLoadedAssemblies = ResolveCurrentAssembliesDelegate(); | ||
| } | ||
|
|
||
| if (_getLoadedAssemblies != null) | ||
| { | ||
| try | ||
| { | ||
| return _getLoadedAssemblies(); | ||
| } | ||
| catch | ||
| { | ||
| // If the new API throws for any reason, fall through to the legacy path. | ||
| } | ||
| } | ||
|
|
||
| return AppDomain.CurrentDomain.GetAssemblies(); | ||
| } | ||
|
|
||
| private static Func<Assembly[]> ResolveCurrentAssembliesDelegate() | ||
| { | ||
| // 1. Try direct AQN lookups first — cheap, side-effect-free, and | ||
| // avoids touching AppDomain.GetAssemblies on CoreCLR. | ||
| foreach (var aqn in CurrentAssembliesAqns) | ||
| { | ||
| Type type; | ||
| try { type = Type.GetType(aqn, throwOnError: false); } | ||
| catch { type = null; } | ||
|
|
||
| var del = TryBindGetLoadedAssemblies(type); | ||
| if (del != null) return del; | ||
| } | ||
|
|
||
| // 2. Fallback: scan every loaded assembly. This still uses the legacy | ||
| // enumeration API but only runs once during the bootstrap probe, | ||
| // and only when none of the AQNs above resolved. | ||
| foreach (var asm in AppDomain.CurrentDomain.GetAssemblies()) | ||
| { | ||
| Type type; | ||
| try { type = asm.GetType("UnityEngine.Assemblies.CurrentAssemblies", throwOnError: false); } | ||
| catch { continue; } | ||
|
|
||
| var del = TryBindGetLoadedAssemblies(type); | ||
| if (del != null) return del; | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| private static Func<Assembly[]> TryBindGetLoadedAssemblies(Type type) | ||
| { | ||
| if (type == null) return null; | ||
|
|
||
| var method = type.GetMethod( | ||
| "GetLoadedAssemblies", | ||
| BindingFlags.Public | BindingFlags.Static, | ||
| null, | ||
| Type.EmptyTypes, | ||
| null); | ||
|
|
||
| if (method == null || !typeof(Assembly[]).IsAssignableFrom(method.ReturnType)) | ||
| return null; | ||
|
|
||
| try | ||
| { | ||
| return (Func<Assembly[]>)Delegate.CreateDelegate(typeof(Func<Assembly[]>), method); | ||
| } | ||
| catch | ||
| { | ||
| return () => (Assembly[])method.Invoke(null, null); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Add coverage for the new compat paths before merge.
UnityAssembliesCompat now sits on the discovery path for CommandRegistry auto-registration and UnityTypeResolver, and this PR also changes UnityFindObjectsCompat's version routing. I don't see corresponding tests in the provided changes, so a Unity upgrade can silently hide tools/types or change object lookup behavior without failing CI.
If you want, I can help sketch editor tests that pin both the CurrentAssemblies fallback behavior and the 2022.3/6000.x object-find branches. Based on learnings "Every new feature must include tests before submission".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Runtime/Helpers/UnityAssembliesCompat.cs` around lines 22 - 120,
Add editor tests that pin the new discovery/compat paths so upgrades don't
silently break auto-registration or object lookup: create tests that exercise
UnityAssembliesCompat.GetLoadedAssemblies and ResolveCurrentAssembliesDelegate
(verify CurrentAssembliesAqns entry resolution and the fallback scan path both
return a sensible set including a known test assembly/type) and tests that
exercise UnityFindObjectsCompat's version-routing branches (simulate both 2022.3
and 6000.x behaviors and assert UnityTypeResolver/CommandRegistry
auto-registration finds a known test type and that FindObjects behavior returns
expected objects); if Type.GetType/AQN resolution is unavailable in the test
runner, force the fallback by preloading a test assembly and assert the delegate
returned from TryBindGetLoadedAssemblies behaves the same as
AppDomain.CurrentDomain.GetAssemblies.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MCPForUnity/Editor/Tools/ManageScene.cs (1)
1175-1192:⚠️ Potential issue | 🔴 CriticalSanitize
fileNamebeforePath.Combineto prevent path escape writes.Line 1178 combines user input directly without filtering path separators. An attacker can bypass the output folder constraint by supplying paths like
../../../etc/passwd.png,/root/file.png, orC:\Windows\System32\evil.png, causingFile.WriteAllBytesto write outside the intended directory.Suggested patch
string fileName = !string.IsNullOrEmpty(cmd.fileName) ? (cmd.fileName.EndsWith(".png", System.StringComparison.OrdinalIgnoreCase) ? cmd.fileName : cmd.fileName + ".png") : $"screenshot-{DateTime.Now:yyyyMMdd-HHmmss}.png"; + // Accept file names only (no path segments / rooted paths). + fileName = Path.GetFileName(fileName.Replace('\\', '/')); + if (string.IsNullOrWhiteSpace(fileName)) + return new ErrorResponse("Invalid 'fileName'. Provide a file name only (no path)."); + string fullPath = Path.Combine(folderAbsolute, fileName);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/ManageScene.cs` around lines 1175 - 1192, Sanitize and validate the user-supplied file name before combining it with folderAbsolute: instead of using cmd.fileName directly, derive a safe fileName by stripping path components (use Path.GetFileName-like behavior), remove/replace invalid path characters, ensure it is not rooted, and fall back to the generated timestamp name if empty; after constructing fullPath, resolve its absolute path and verify it starts with the intended folderAbsolute full path to prevent path-escaping, then proceed with the unique-filename loop and File.WriteAllBytes using those validated variables (refer to variables/methods fileName, cmd.fileName, folderAbsolute, fullPath).
🧹 Nitpick comments (1)
MCPForUnity/Editor/Tools/ManageScene.cs (1)
1194-1200: ⚡ Quick winUse
ScreenshotUtility.ToProjectRelativePath(...)instead of duplicating root math.This block duplicates project-root normalization already provided by the shared helper, which risks drift.
Suggested patch
- string projectRoot = Path.GetFullPath(Path.Combine(Application.dataPath, "..")).Replace('\\', '/'); - string normalizedFull = fullPath.Replace('\\', '/'); - string normalizedRoot = projectRoot.EndsWith("/") ? projectRoot : projectRoot + "/"; - string projectRelativePath = normalizedFull.StartsWith(normalizedRoot, StringComparison.OrdinalIgnoreCase) - ? normalizedFull.Substring(normalizedRoot.Length) - : normalizedFull; + string normalizedFull = fullPath.Replace('\\', '/'); + string projectRelativePath = ScreenshotUtility.ToProjectRelativePath(normalizedFull);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/ManageScene.cs` around lines 1194 - 1200, Replace the manual project-root normalization in ManageScene (the block that computes projectRoot, normalizedFull, normalizedRoot and projectRelativePath from fullPath) with the shared helper; call ScreenshotUtility.ToProjectRelativePath(fullPath) to get projectRelativePath instead of duplicating logic so you use the centralized normalization routine and avoid drift.
🤖 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/Tools/ManageScene.cs`:
- Line 113: The code extracts outputFolder directly from the JObject
(p["outputFolder"] ?? p["output_folder"]) in ManageScene; replace that direct
access with the centralized ToolParams extraction used elsewhere—instantiate or
use the existing ToolParams for this handler and call its string/param getter
for "output_folder"/"outputFolder" (the same key used by other handlers) so
parameter normalization and validation are applied consistently to outputFolder.
---
Outside diff comments:
In `@MCPForUnity/Editor/Tools/ManageScene.cs`:
- Around line 1175-1192: Sanitize and validate the user-supplied file name
before combining it with folderAbsolute: instead of using cmd.fileName directly,
derive a safe fileName by stripping path components (use Path.GetFileName-like
behavior), remove/replace invalid path characters, ensure it is not rooted, and
fall back to the generated timestamp name if empty; after constructing fullPath,
resolve its absolute path and verify it starts with the intended folderAbsolute
full path to prevent path-escaping, then proceed with the unique-filename loop
and File.WriteAllBytes using those validated variables (refer to
variables/methods fileName, cmd.fileName, folderAbsolute, fullPath).
---
Nitpick comments:
In `@MCPForUnity/Editor/Tools/ManageScene.cs`:
- Around line 1194-1200: Replace the manual project-root normalization in
ManageScene (the block that computes projectRoot, normalizedFull, normalizedRoot
and projectRelativePath from fullPath) with the shared helper; call
ScreenshotUtility.ToProjectRelativePath(fullPath) to get projectRelativePath
instead of duplicating logic so you use the centralized normalization routine
and avoid drift.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b1d06427-3da7-409b-a2f3-7126454e5a72
📒 Files selected for processing (5)
MCPForUnity/Editor/Tools/ManageScene.csMCPForUnity/Editor/Tools/ManageUI.csMCPForUnity/Editor/Windows/Components/Advanced/McpAdvancedSection.csMCPForUnity/Runtime/Helpers/ScreenshotUtility.csServer/src/services/tools/utils.py
🚧 Files skipped from review as they are similar to previous changes (2)
- Server/src/services/tools/utils.py
- MCPForUnity/Editor/Tools/ManageUI.cs
| captureSource = toolParams.Get("capture_source"), | ||
| includeImage = ParamCoercion.CoerceBoolNullable(p["includeImage"] ?? p["include_image"]), | ||
| maxResolution = ParamCoercion.CoerceIntNullable(p["maxResolution"] ?? p["max_resolution"]), | ||
| outputFolder = (p["outputFolder"] ?? p["output_folder"])?.ToString(), |
Contributor
There was a problem hiding this comment.
Use ToolParams for output_folder extraction to keep tool-param handling consistent.
The new outputFolder path is parsed directly from JObject; this should go through ToolParams like other normalized tool params.
Suggested patch
- outputFolder = (p["outputFolder"] ?? p["output_folder"])?.ToString(),
+ outputFolder = toolParams.Get("output_folder"),As per coding guidelines: "C# tool handlers must use ToolParams class for consistent parameter validation and extraction with support for both snake_case and camelCase parameter names".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Tools/ManageScene.cs` at line 113, The code extracts
outputFolder directly from the JObject (p["outputFolder"] ?? p["output_folder"])
in ManageScene; replace that direct access with the centralized ToolParams
extraction used elsewhere—instantiate or use the existing ToolParams for this
handler and call its string/param getter for "output_folder"/"outputFolder" (the
same key used by other handlers) so parameter normalization and validation are
applied consistently to outputFolder.
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.
1.Add Compat-based scripts revolving around UnityCompatShims.cs, that will document our current API Compatibility changes in several files. For #1091 and other potential issues
2.Add custom screenshot folder selection
Summary by CodeRabbit
New Features
Improvements