Plugin eviction, CLI camera/graphics, minor fixes#911
Merged
Scriptwonder merged 2 commits intoCoplayDev:betafrom Mar 10, 2026
Merged
Plugin eviction, CLI camera/graphics, minor fixes#911Scriptwonder merged 2 commits intoCoplayDev:betafrom
Scriptwonder merged 2 commits intoCoplayDev:betafrom
Conversation
Handle plugin session evictions and extend CLI/docs; misc Unity tooling fixes. - Server: PluginRegistry.register now returns (session, evicted_session_id). PluginHub consumes the evicted id to cancel ping tasks, clear pending commands, remove state, and close the old WebSocket to avoid orphaned connections after reconnection races. Tests updated to unpack the returned tuple. - CLI/docs: Add extensive camera, graphics, packages, and texture command examples and raw command snippets across CLI guides and examples; update command reference tables. - Unity editor tools: SkyboxOps adds a CustomReflectionTexture accessor to handle Unity API differences (RenderSettings.customReflection vs customReflectionTexture). ManageScript now ignores C# keywords when detecting duplicate methods to prevent false positives; added unit test to cover keyword cases. - Misc: Small docstring tweaks in services/tools/utils.py and skill docs updated to reference manage_camera screenshots.
Contributor
Reviewer's GuideImplements proper cleanup of evicted Unity plugin sessions during registration, expands CLI/docs with detailed camera/graphics/package/texture command examples and raw usages, and hardens Unity editor tooling for skybox reflection handling and script duplicate-method detection (with new tests). Sequence diagram for plugin session registration and eviction handlingsequenceDiagram
actor UnityPlugin
participant PluginHub
participant PluginRegistry
participant Connections
participant PingTasks
participant PendingCommands
participant EvictedWS
UnityPlugin->>PluginHub: RegisterMessage(session_id, project_hash, user_id, ...)
PluginHub->>UnityPlugin: RegisteredMessage(session_id)
PluginHub->>PluginRegistry: register(session_id, project_hash, user_id, ...)
PluginRegistry-->>PluginHub: session, evicted_session_id
rect rgb(235,235,255)
alt evicted_session_id is not None
PluginHub->>Connections: pop(evicted_session_id) as evicted_ws
PluginHub->>PingTasks: pop(evicted_session_id) as old_ping
alt old_ping exists and not done
PluginHub->>PingTasks: old_ping.cancel()
end
PluginHub->>PluginHub: remove last_pong[evicted_session_id]
PluginHub->>PendingCommands: find entries where session_id == evicted_session_id
loop each pending command for evicted_session_id
PluginHub->>PendingCommands: future.set_exception(PluginDisconnectedError)
PluginHub->>PendingCommands: remove(command_id)
end
else no previous session
PluginHub->>PluginHub: skip eviction cleanup
end
end
PluginHub->>Connections: connections[session.session_id] = websocket
PluginHub->>PluginHub: last_pong[session_id] = now
PluginHub->>PingTasks: ping_tasks[session_id] = create_task(ping_loop)
alt evicted_ws is not None
PluginHub->>EvictedWS: close(evicted_ws, code=1001)
end
Class diagram for PluginRegistry, PluginHub, SkyboxOps, and ManageScript changesclassDiagram
class PluginSession {
+session_id : str
+project_name : str
+project_hash : str
+unity_version : str
+project_path : str
+user_id : str
+connected_at : datetime
}
class PluginRegistry {
-_sessions : dict~str, PluginSession~
-_hash_to_session : dict~str, str~
-_user_hash_to_session : dict~tuple~str, str~, str~
+async register(session_id, project_name, project_hash, unity_version, project_path, user_id) : (PluginSession, str)
+async touch(session_id) : None
}
class PluginHub {
<<async server>>
+_connections : dict~str, WebSocket~
+_ping_tasks : dict~str, Task~
+_last_pong : dict~str, float~
+_pending : dict~str, dict~str, any~~
+async _handle_register(websocket, payload) : None
+async _ping_loop(session_id, websocket) : None
}
class PluginDisconnectedError {
+message : str
}
PluginRegistry --> PluginSession : manages
PluginHub ..> PluginRegistry : uses register
PluginHub ..> PluginDisconnectedError : raises on eviction
class SkyboxOps {
<<static>>
+Texture CustomReflectionTexture
+object GetEnvironment(JObject params)
+object SetReflection(JObject params)
}
SkyboxOps ..> Texture : uses
SkyboxOps ..> RenderSettings : reads_writes
SkyboxOps ..> AssetDatabase : uses
class ManageScript {
+void CheckDuplicateMethodSignatures(string contents, IList warnings)
-static HashSet~string~ CSharpKeywords
-static bool IsCSharpKeyword(string name)
-static int CountTopLevelParams(string paramList)
-static string ExtractParamTypes(string paramList)
}
ManageScript ..> System_Collections_Generic_HashSet : uses
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughThe PR introduces cross-version compatibility for Unity reflection texture access in SkyboxOps.cs, enhances duplicate method detection by filtering C# keywords, implements session eviction tracking in the plugin registry with cleanup procedures, and significantly expands CLI/documentation with command examples for camera, graphics, package, and texture operations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PluginHub
participant PluginRegistry
participant OldSession
participant NewSession
participant Connections
Client->>PluginHub: register(new_plugin)
PluginHub->>PluginRegistry: register(new_plugin)
PluginRegistry->>PluginRegistry: Check for existing session
alt Existing session found
PluginRegistry->>OldSession: Mark as evicted
PluginRegistry->>PluginRegistry: Remove old session from _sessions
PluginRegistry-->>PluginHub: (new_session, evicted_session_id)
else No existing session
PluginRegistry-->>PluginHub: (new_session, None)
end
PluginHub->>Connections: Get old WebSocket (if evicted)
PluginHub->>OldSession: Cancel ping loop
PluginHub->>OldSession: Clear last_pong
PluginHub->>OldSession: Fail in-flight commands with PluginDisconnectedError
PluginHub->>Connections: Close old WebSocket (outside lock)
PluginHub->>NewSession: Start new ping loop
PluginHub->>Connections: Track new session
PluginHub-->>Client: Registration complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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:
- When evicting a previous session in
PluginHub._handle_register, consider logging the affectedcommand_idvalues when setting exceptions on pending futures to make it easier to trace which in-flight commands were cancelled during reconnection races. - The new CLI camera/graphics/packages/texture examples are duplicated across
CLI_USAGE_GUIDE.md,CLI_USAGE.md, andCLI_EXAMPLE.mdwith slight differences (e.g.,PostFXvsPostProcesstargets and option variations); it may be worth standardizing the examples or factoring common snippets to avoid drift and user confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When evicting a previous session in `PluginHub._handle_register`, consider logging the affected `command_id` values when setting exceptions on pending futures to make it easier to trace which in-flight commands were cancelled during reconnection races.
- The new CLI camera/graphics/packages/texture examples are duplicated across `CLI_USAGE_GUIDE.md`, `CLI_USAGE.md`, and `CLI_EXAMPLE.md` with slight differences (e.g., `PostFX` vs `PostProcess` targets and option variations); it may be worth standardizing the examples or factoring common snippets to avoid drift and user confusion.
## Individual Comments
### Comment 1
<location path="Server/src/transport/plugin_hub.py" line_range="440-442" />
<code_context>
+ if old_ping and not old_ping.done():
+ old_ping.cancel()
+ cls._last_pong.pop(evicted_session_id, None)
+ for command_id, entry in list(cls._pending.items()):
+ if entry.get("session_id") == evicted_session_id:
+ future = entry.get("future") if isinstance(entry, dict) else None
+ if future and not future.done():
+ future.set_exception(
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard use of `entry.get` when `entry` may not be a dict
Here `entry.get("session_id")` is used before you check `isinstance(entry, dict)` for the `future` access. If `_pending` can contain non-dict entries, this first call to `.get()` will raise `AttributeError`. Either ensure `_pending` only stores dicts, or add the same `isinstance(entry, dict)` guard (or equivalent normalization/skip logic) around the `session_id` access.
</issue_to_address>
### Comment 2
<location path="Server/src/transport/plugin_hub.py" line_range="463-468" />
<code_context>
ping_task = asyncio.create_task(cls._ping_loop(session_id, websocket))
cls._ping_tasks[session_id] = ping_task
+ # Close evicted WebSocket outside the lock to avoid blocking
+ if evicted_ws is not None:
+ try:
+ await evicted_ws.close(code=1001)
+ except Exception:
+ pass
+
</code_context>
<issue_to_address>
**suggestion:** Consider logging failures when closing the evicted WebSocket
The broad `except Exception: pass` around `evicted_ws.close(...)` hides any errors when closing the previous connection, which makes diagnosing reconnection/eviction issues difficult. Consider logging the exception (e.g., at debug level) while still swallowing it so errors aren’t propagated to the caller.
```suggestion
# Close evicted WebSocket outside the lock to avoid blocking
if evicted_ws is not None:
try:
await evicted_ws.close(code=1001)
except Exception:
logger.debug(
"Failed to close evicted WebSocket for session %s",
evicted_session_id,
exc_info=True,
)
```
</issue_to_address>
### Comment 3
<location path="Server/tests/integration/test_plugin_registry_user_isolation.py" line_range="13-16" />
<code_context>
async def test_register_with_user_id_stores_composite_key(self):
registry = PluginRegistry()
- session = await registry.register(
+ session, _ = await registry.register(
"sess-1", "MyProject", "hash1", "2022.3", user_id="user-A"
)
</code_context>
<issue_to_address>
**suggestion (testing):** Add integration tests asserting `evicted_session_id` behavior for user-scoped registrations
Since `PluginRegistry.register` now returns `(session, evicted_session_id)`, this suite should assert the eviction semantics instead of discarding the second value:
- Same `user_id` and `project_hash`: the second `register` with a new `session_id` should return `evicted_session_id == first_session_id`.
- Different `user_id` values with the same `project_hash`: both calls should return `evicted_session_id is None`.
These expectations will lock in the user-scoped eviction behavior rather than leaving it untested.
Suggested implementation:
```python
@pytest.mark.asyncio
async def test_register_with_user_id_stores_composite_key(self):
registry = PluginRegistry()
session, _ = await registry.register(
"sess-1", "MyProject", "hash1", "2022.3", user_id="user-A"
)
assert session.user_id == "user-A"
@pytest.mark.asyncio
async def test_register_same_user_same_hash_evicts_previous_session(self):
"""Same user + project_hash: second registration evicts the first session."""
registry = PluginRegistry()
first_session, first_evicted_session_id = await registry.register(
"sess-1", "MyProject", "hash1", "2022.3", user_id="user-A"
)
assert first_session.session_id == "sess-1"
assert first_evicted_session_id is None
second_session, second_evicted_session_id = await registry.register(
"sess-2", "MyProject", "hash1", "2022.3", user_id="user-A"
)
assert second_session.session_id == "sess-2"
assert second_evicted_session_id == "sess-1"
async def test_cross_user_isolation_same_hash(self):
```
```python
async def test_cross_user_isolation_same_hash(self):
"""Two users registering with the same project_hash get independent sessions."""
registry = PluginRegistry()
sess_a, evicted_a = await registry.register(
"sA", "Proj", "hash1", "2022", user_id="userA"
)
sess_b, evicted_b = await registry.register(
"sB", "Proj", "hash1", "2022", user_id="userB"
)
assert sess_a.session_id == "sA"
assert sess_b.session_id == "sB"
# Different users should not evict each other's sessions
assert evicted_a is None
assert evicted_b is None
```
</issue_to_address>
### Comment 4
<location path="Server/src/cli/CLI_USAGE_GUIDE.md" line_range="816-820" />
<code_context>
unity-mcp raw manage_components '{"action": "add", "target": "Test", "componentType": "Rigidbody"}'
unity-mcp raw manage_editor '{"action": "play"}'
+unity-mcp raw manage_camera '{"action": "screenshot", "include_image": true}'
+unity-mcp raw manage_graphics '{"action": "volume_info", "target": "PostProcess"}'
+unity-mcp raw manage_packages '{"action": "list"}'
```
</code_context>
<issue_to_address>
**suggestion (typo):** Volume name in raw example appears inconsistent with earlier "PostProcessing" examples.
Above, the volume is created and used as `"PostProcessing"`, but this example uses `"PostProcess"`. If it’s the same volume, please update the target here to `"PostProcessing"` for consistency.
```suggestion
unity-mcp raw manage_components '{"action": "add", "target": "Test", "componentType": "Rigidbody"}'
unity-mcp raw manage_editor '{"action": "play"}'
+unity-mcp raw manage_camera '{"action": "screenshot", "include_image": true}'
+unity-mcp raw manage_graphics '{"action": "volume_info", "target": "PostProcessing"}'
+unity-mcp raw manage_packages '{"action": "list"}'
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Server/src/transport/plugin_hub.py
Outdated
Comment on lines
+440
to
+442
| for command_id, entry in list(cls._pending.items()): | ||
| if entry.get("session_id") == evicted_session_id: | ||
| future = entry.get("future") if isinstance(entry, dict) else None |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Guard use of entry.get when entry may not be a dict
Here entry.get("session_id") is used before you check isinstance(entry, dict) for the future access. If _pending can contain non-dict entries, this first call to .get() will raise AttributeError. Either ensure _pending only stores dicts, or add the same isinstance(entry, dict) guard (or equivalent normalization/skip logic) around the session_id access.
Comment on lines
+13
to
16
| session, _ = await registry.register( | ||
| "sess-1", "MyProject", "hash1", "2022.3", user_id="user-A" | ||
| ) | ||
| assert session.user_id == "user-A" |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add integration tests asserting evicted_session_id behavior for user-scoped registrations
Since PluginRegistry.register now returns (session, evicted_session_id), this suite should assert the eviction semantics instead of discarding the second value:
- Same
user_idandproject_hash: the secondregisterwith a newsession_idshould returnevicted_session_id == first_session_id. - Different
user_idvalues with the sameproject_hash: both calls should returnevicted_session_id is None.
These expectations will lock in the user-scoped eviction behavior rather than leaving it untested.
Suggested implementation:
@pytest.mark.asyncio
async def test_register_with_user_id_stores_composite_key(self):
registry = PluginRegistry()
session, _ = await registry.register(
"sess-1", "MyProject", "hash1", "2022.3", user_id="user-A"
)
assert session.user_id == "user-A"
@pytest.mark.asyncio
async def test_register_same_user_same_hash_evicts_previous_session(self):
"""Same user + project_hash: second registration evicts the first session."""
registry = PluginRegistry()
first_session, first_evicted_session_id = await registry.register(
"sess-1", "MyProject", "hash1", "2022.3", user_id="user-A"
)
assert first_session.session_id == "sess-1"
assert first_evicted_session_id is None
second_session, second_evicted_session_id = await registry.register(
"sess-2", "MyProject", "hash1", "2022.3", user_id="user-A"
)
assert second_session.session_id == "sess-2"
assert second_evicted_session_id == "sess-1"
async def test_cross_user_isolation_same_hash(self): async def test_cross_user_isolation_same_hash(self):
"""Two users registering with the same project_hash get independent sessions."""
registry = PluginRegistry()
sess_a, evicted_a = await registry.register(
"sA", "Proj", "hash1", "2022", user_id="userA"
)
sess_b, evicted_b = await registry.register(
"sB", "Proj", "hash1", "2022", user_id="userB"
)
assert sess_a.session_id == "sA"
assert sess_b.session_id == "sB"
# Different users should not evict each other's sessions
assert evicted_a is None
assert evicted_b is None![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docs/guides/CLI_USAGE.md (4)
819-820:⚠️ Potential issue | 🔴 CriticalIncorrect action names in raw command examples
Lines 819-820 use incorrect internal action names:
- Line 819: Uses
"action": "volume_info"but the correct action name is"volume_get_info"(see MCPForUnity/Editor/Tools/Graphics/ManageGraphics.cs)- Line 820: Uses
"action": "list"but the correct action name is"list_packages"(see MCPForUnity/Editor/Tools/ManagePackages.cs)Raw commands bypass the CLI's action name normalization and must use the exact internal action names that the C# handlers expect.
🐛 Proposed fix
-unity-mcp raw manage_graphics '{"action": "volume_info", "target": "PostProcess"}' -unity-mcp raw manage_packages '{"action": "list"}' +unity-mcp raw manage_graphics '{"action": "volume_get_info", "target": "PostProcess"}' +unity-mcp raw manage_packages '{"action": "list_packages"}'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guides/CLI_USAGE.md` around lines 819 - 820, Update the raw JSON command examples to use the exact internal action names expected by the C# handlers: replace "action": "volume_info" with "action": "volume_get_info" (handler referenced in ManageGraphics.cs) and replace "action": "list" with "action": "list_packages" (handler referenced in ManagePackages.cs); ensure any raw command examples elsewhere follow the same exact internal action name convention so they match the C# action handlers.
74-74:⚠️ Potential issue | 🟡 MinorParameter name inconsistency:
--camera-refvs--cameraLine 74 uses
--camera-ref "SecondCamera"but the Screenshot Parameters table at line 190 documents the parameter as--camera, -c. Please use consistent parameter naming throughout the documentation.📝 Suggested fix
-unity-mcp camera screenshot --camera-ref "SecondCamera" --include-image +unity-mcp camera screenshot --camera "SecondCamera" --include-image🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guides/CLI_USAGE.md` at line 74, The CLI example uses the parameter --camera-ref while the Screenshot Parameters table documents --camera, -c; make these consistent by either updating the example to use --camera (e.g., replace --camera-ref "SecondCamera" with --camera "SecondCamera") or by updating the Screenshot Parameters table to document --camera-ref (and include its short form if applicable), and ensure every occurrence of the flag in the document (search for --camera-ref and --camera) uses the chosen canonical name so examples and the table match.
658-658:⚠️ Potential issue | 🟡 MinorParameter name inconsistency:
--camera-refvs--cameraLine 658 uses
--camera-ref "SecondCamera"but docs/guides/CLI_USAGE.md line 190 documents this parameter as--camera, -c. Please ensure consistent parameter naming across all documentation files.📝 Suggested fix
-unity-mcp camera screenshot --camera-ref "SecondCamera" --include-image +unity-mcp camera screenshot --camera "SecondCamera" --include-image🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guides/CLI_USAGE.md` at line 658, Documentation uses two different CLI flag names (--camera-ref vs --camera / -c); standardize to the documented flag set (--camera, -c). Search for occurrences of "--camera-ref" and replace them with "--camera" (and add the short form "-c" where examples show only the long flag), and update example invocation text to match the flag description in the CLI_USAGE.md CLI parameter section and any examples that reference "SecondCamera" to use the unified flag name; ensure all README/examples and guide references consistently use "--camera" and "-c".
233-234:⚠️ Potential issue | 🔴 CriticalIncorrect action names in raw command examples
Lines 233-234 use incorrect internal action names:
- Line 233: Uses
"action":"volume_info"but the correct action name is"volume_get_info"(see MCPForUnity/Editor/Tools/Graphics/ManageGraphics.cs)- Line 234: Uses
"action":"list"but the correct action name is"list_packages"(see MCPForUnity/Editor/Tools/ManagePackages.cs)Raw commands must use the exact internal action names that the C# handlers expect.
🐛 Proposed fix
-unity-mcp raw manage_graphics '{"action":"volume_info","target":"PostFX"}' -unity-mcp raw manage_packages '{"action":"list"}' +unity-mcp raw manage_graphics '{"action":"volume_get_info","target":"PostFX"}' +unity-mcp raw manage_packages '{"action":"list_packages"}'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guides/CLI_USAGE.md` around lines 233 - 234, Update the raw command examples to use the exact internal action names expected by the C# handlers: replace any occurrences of "volume_info" with "volume_get_info" (handled by ManageGraphics) and replace "list" with "list_packages" (handled by ManagePackages); ensure the example command strings (e.g., the unity-mcp examples under Particle systems) are updated so the action fields match volume_get_info and list_packages exactly.
🧹 Nitpick comments (2)
Server/src/transport/plugin_hub.py (1)
463-468: Consider logging the exception instead of silently swallowing it.While best-effort cleanup is appropriate here (the WebSocket may already be closed or in a bad state), silently ignoring exceptions can mask unexpected issues during debugging.
🔧 Suggested improvement
# Close evicted WebSocket outside the lock to avoid blocking if evicted_ws is not None: try: await evicted_ws.close(code=1001) except Exception: - pass + logger.debug( + "Error closing evicted WebSocket for session %s (best-effort cleanup)", + evicted_session_id, + exc_info=True, + )This matches the pattern used in
_evict_connectionat line 762 which also logs close errors at debug level.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/transport/plugin_hub.py` around lines 463 - 468, The close() call for evicted_ws currently swallows all exceptions; update the except block to log the exception at debug level (like the pattern in _evict_connection) rather than silently ignoring it. Catch Exception as e and call the same logger used in _evict_connection (e.g., logger.debug or self.logger.debug) with a message such as "error closing evicted websocket" and include the exception info to preserve traceback/context. Ensure the change only affects the small try/except around evicted_ws.close and keeps the best-effort cleanup behavior.unity-mcp-skill/SKILL.md (1)
93-96: Consider clarifying the relationship betweenaction="screenshot_multiview"andaction="screenshot", batch="surround".Line 95 shows
action="screenshot_multiview"for a 6-angle contact sheet, while lines 71-74 useaction="screenshot"withbatch="surround"to achieve the same result. Both approaches are valid—screenshot_multiviewis a built-in shorthand forscreenshotwithbatch='surround'andinclude_image=true.For improved clarity, consider either:
- Adding a note explaining that
screenshot_multiviewis equivalent to the batch parameter approach shown earlier, or- Using only one approach consistently throughout the document.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@unity-mcp-skill/SKILL.md` around lines 93 - 96, Clarify that action="screenshot_multiview" is a shorthand for manage_camera(action="screenshot", batch="surround", include_image=True) by adding a short note near the examples (or pick one style and make the other a footnote) so readers know both produce the same 6-angle contact sheet; reference the examples using action="screenshot_multiview" and the earlier manage_camera(action="screenshot", batch="surround") example to show equivalence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/guides/CLI_USAGE.md`:
- Around line 468-469: Update the raw CLI examples to use the exact internal
action names expected by the C# handlers: change the manage_graphics example to
use "action": "volume_get_info" (handler in ManageGraphics) instead of
"volume_info", and change the manage_packages example to use "action":
"list_packages" (handler in ManagePackages) instead of "list"; keep the
surrounding command syntax unchanged so the raw invocation matches the internal
action names.
---
Outside diff comments:
In `@docs/guides/CLI_USAGE.md`:
- Around line 819-820: Update the raw JSON command examples to use the exact
internal action names expected by the C# handlers: replace "action":
"volume_info" with "action": "volume_get_info" (handler referenced in
ManageGraphics.cs) and replace "action": "list" with "action": "list_packages"
(handler referenced in ManagePackages.cs); ensure any raw command examples
elsewhere follow the same exact internal action name convention so they match
the C# action handlers.
- Line 74: The CLI example uses the parameter --camera-ref while the Screenshot
Parameters table documents --camera, -c; make these consistent by either
updating the example to use --camera (e.g., replace --camera-ref "SecondCamera"
with --camera "SecondCamera") or by updating the Screenshot Parameters table to
document --camera-ref (and include its short form if applicable), and ensure
every occurrence of the flag in the document (search for --camera-ref and
--camera) uses the chosen canonical name so examples and the table match.
- Line 658: Documentation uses two different CLI flag names (--camera-ref vs
--camera / -c); standardize to the documented flag set (--camera, -c). Search
for occurrences of "--camera-ref" and replace them with "--camera" (and add the
short form "-c" where examples show only the long flag), and update example
invocation text to match the flag description in the CLI_USAGE.md CLI parameter
section and any examples that reference "SecondCamera" to use the unified flag
name; ensure all README/examples and guide references consistently use
"--camera" and "-c".
- Around line 233-234: Update the raw command examples to use the exact internal
action names expected by the C# handlers: replace any occurrences of
"volume_info" with "volume_get_info" (handled by ManageGraphics) and replace
"list" with "list_packages" (handled by ManagePackages); ensure the example
command strings (e.g., the unity-mcp examples under Particle systems) are
updated so the action fields match volume_get_info and list_packages exactly.
---
Nitpick comments:
In `@Server/src/transport/plugin_hub.py`:
- Around line 463-468: The close() call for evicted_ws currently swallows all
exceptions; update the except block to log the exception at debug level (like
the pattern in _evict_connection) rather than silently ignoring it. Catch
Exception as e and call the same logger used in _evict_connection (e.g.,
logger.debug or self.logger.debug) with a message such as "error closing evicted
websocket" and include the exception info to preserve traceback/context. Ensure
the change only affects the small try/except around evicted_ws.close and keeps
the best-effort cleanup behavior.
In `@unity-mcp-skill/SKILL.md`:
- Around line 93-96: Clarify that action="screenshot_multiview" is a shorthand
for manage_camera(action="screenshot", batch="surround", include_image=True) by
adding a short note near the examples (or pick one style and make the other a
footnote) so readers know both produce the same 6-angle contact sheet; reference
the examples using action="screenshot_multiview" and the earlier
manage_camera(action="screenshot", batch="surround") example to show
equivalence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e1109109-7364-4efb-97b8-8a1afd0342ee
📒 Files selected for processing (12)
MCPForUnity/Editor/Tools/Graphics/SkyboxOps.csMCPForUnity/Editor/Tools/ManageScript.csServer/src/cli/CLI_USAGE_GUIDE.mdServer/src/services/tools/utils.pyServer/src/transport/plugin_hub.pyServer/src/transport/plugin_registry.pyServer/tests/integration/test_plugin_registry_user_isolation.pyServer/tests/test_transport_characterization.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptValidationTests.csdocs/guides/CLI_EXAMPLE.mddocs/guides/CLI_USAGE.mdunity-mcp-skill/SKILL.md
docs/guides/CLI_USAGE.md
Outdated
Comment on lines
+468
to
+469
| unity-mcp raw manage_graphics '{"action": "volume_info", "target": "PostFX"}' | ||
| unity-mcp raw manage_packages '{"action": "list"}' |
Contributor
There was a problem hiding this comment.
Incorrect action names in raw command examples
Lines 468-469 use incorrect internal action names:
- Line 468: Uses
"action": "volume_info"but the correct action name is"volume_get_info"(see MCPForUnity/Editor/Tools/Graphics/ManageGraphics.cs) - Line 469: Uses
"action": "list"but the correct action name is"list_packages"(see MCPForUnity/Editor/Tools/ManagePackages.cs)
Raw commands bypass the CLI's action name normalization and must use the exact internal action names that the C# handlers expect.
🐛 Proposed fix
-unity-mcp raw manage_graphics '{"action": "volume_info", "target": "PostFX"}'
-unity-mcp raw manage_packages '{"action": "list"}'
+unity-mcp raw manage_graphics '{"action": "volume_get_info", "target": "PostFX"}'
+unity-mcp raw manage_packages '{"action": "list_packages"}'📝 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
| unity-mcp raw manage_graphics '{"action": "volume_info", "target": "PostFX"}' | |
| unity-mcp raw manage_packages '{"action": "list"}' | |
| unity-mcp raw manage_graphics '{"action": "volume_get_info", "target": "PostFX"}' | |
| unity-mcp raw manage_packages '{"action": "list_packages"}' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/guides/CLI_USAGE.md` around lines 468 - 469, Update the raw CLI examples
to use the exact internal action names expected by the C# handlers: change the
manage_graphics example to use "action": "volume_get_info" (handler in
ManageGraphics) instead of "volume_info", and change the manage_packages example
to use "action": "list_packages" (handler in ManagePackages) instead of "list";
keep the surrounding command syntax unchanged so the raw invocation matches the
internal action names.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses Unity plugin reconnection races by explicitly evicting superseded plugin sessions on the server, while also extending CLI/skill documentation with camera/graphics/packages/texture examples and applying a couple of Unity editor tooling fixes (Skybox reflection API compatibility + ManageScript duplicate-method detection improvements).
Changes:
- Server transport:
PluginRegistry.register()now returns(session, evicted_session_id), andPluginHubuses the evicted ID to cancel ping tasks, fail pending commands, remove old state, and close the old WebSocket. - Unity editor tools: add a Unity-version-compatible accessor for custom reflection textures and ignore C# keywords during duplicate-method signature detection (plus a unit test).
- Docs/CLI: add/expand camera/graphics/packages/texture command examples and raw command snippets across guides.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
Server/src/transport/plugin_registry.py |
Returns (new_session, evicted_session_id) to enable upstream eviction cleanup. |
Server/src/transport/plugin_hub.py |
Cleans up evicted session state/tasks/pending commands and closes old WebSocket. |
Server/tests/test_transport_characterization.py |
Updates tests to unpack the new (session, evicted_id) return value where needed. |
Server/tests/integration/test_plugin_registry_user_isolation.py |
Updates tests to unpack the new return value where needed. |
MCPForUnity/Editor/Tools/Graphics/SkyboxOps.cs |
Adds CustomReflectionTexture shim for Unity API differences. |
MCPForUnity/Editor/Tools/ManageScript.cs |
Skips C# keywords in duplicate-method signature detection. |
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageScriptValidationTests.cs |
Adds test coverage for keyword-related duplicate detection behavior. |
Server/src/services/tools/utils.py |
Docstring tweaks for shared screenshot helpers. |
docs/guides/CLI_USAGE.md |
Adds camera/graphics/packages/texture sections + raw command examples + command table updates. |
docs/guides/CLI_EXAMPLE.md |
Adds example snippets for camera/graphics/packages/texture + raw command examples. |
Server/src/cli/CLI_USAGE_GUIDE.md |
Adds extended CLI usage examples for camera/graphics/packages/texture + raw command snippets. |
unity-mcp-skill/SKILL.md |
Updates skill guidance to reference manage_camera(action="screenshot") for verification. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+429
to
431
| session, evicted_session_id = await registry.register(session_id, project_name, project_hash, unity_version, project_path, user_id=user_id) | ||
| evicted_ws = None | ||
| async with lock: |
There was a problem hiding this comment.
Potential deadlock due to lock-order inversion between PluginHub’s _lock and PluginRegistry’s internal _lock: _handle_register awaits registry.register(...) before acquiring PluginHub._lock, while on_disconnect acquires PluginHub._lock and then awaits registry.unregister(...) (which takes the registry lock). If these happen concurrently, each task can end up waiting on the other lock. To avoid this, enforce a consistent acquisition order (e.g., never await PluginRegistry methods while holding PluginHub._lock, or acquire PluginHub._lock first in _handle_register and keep registry calls within that order).
Suggested change
| session, evicted_session_id = await registry.register(session_id, project_name, project_hash, unity_version, project_path, user_id=user_id) | |
| evicted_ws = None | |
| async with lock: | |
| evicted_ws = None | |
| async with lock: | |
| session, evicted_session_id = await registry.register( | |
| session_id, | |
| project_name, | |
| project_hash, | |
| unity_version, | |
| project_path, | |
| user_id=user_id, | |
| ) |
Server/src/transport/plugin_hub.py
Outdated
Comment on lines
+441
to
+442
| if entry.get("session_id") == evicted_session_id: | ||
| future = entry.get("future") if isinstance(entry, dict) else None |
There was a problem hiding this comment.
In the eviction cleanup loop, entry.get("session_id") is accessed before the isinstance(entry, dict) check, so the type-guard is ineffective and can’t prevent an AttributeError if a non-dict ever ends up in _pending. Either drop the isinstance check (since _pending is typed/stored as dict entries) or move the type check before any .get(...) access for consistency with the intended safety.
Suggested change
| if entry.get("session_id") == evicted_session_id: | |
| future = entry.get("future") if isinstance(entry, dict) else None | |
| if isinstance(entry, dict) and entry.get("session_id") == evicted_session_id: | |
| future = entry.get("future") |
| unity-mcp graphics feature-list | ||
| unity-mcp graphics feature-add --type "ScreenSpaceAmbientOcclusion" | ||
| unity-mcp graphics feature-configure --name "SSAO" -p Intensity 1.5 | ||
| unity-mcp graphics feature-toggle --name "SSAO" --active|--inactive |
There was a problem hiding this comment.
unity-mcp graphics feature-toggle --name "SSAO" --active|--inactive reads like a literal pipe in a single command, which isn’t valid CLI syntax. Consider splitting into two example lines (one with --active, one with --inactive) or using a clearer optional/alternatives notation so users don’t copy-paste an invalid command.
Suggested change
| unity-mcp graphics feature-toggle --name "SSAO" --active|--inactive | |
| unity-mcp graphics feature-toggle --name "SSAO" --active | |
| unity-mcp graphics feature-toggle --name "SSAO" --inactive |
Comment on lines
+314
to
+315
| typeof(int); | ||
| typeof(string); |
There was a problem hiding this comment.
The snippet inside the new test includes typeof(int); / typeof(string); as standalone statements, which is not valid C# (expression statements can’t be plain typeof expressions). Even though the test currently only exercises ValidationLevel.Basic (so it won’t catch syntax errors), using valid C# here will make the test more robust if validation levels or checks change. Consider assigning the result (e.g., var _ = typeof(int);) or removing these lines.
Suggested change
| typeof(int); | |
| typeof(string); | |
| var _ = typeof(int); | |
| var __ = typeof(string); |
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.
Handle plugin session evictions and extend CLI/docs; misc Unity tooling fixes.
Type of Change
Save your change type
Documentation Updates
tools/UPDATE_DOCS_PROMPT.md(recommended)tools/UPDATE_DOCS.mdRelated Issues
Fixes #910
Summary by Sourcery
Handle plugin session eviction on re-registration, expand CLI camera/graphics/packages/texture documentation and examples, and improve Unity editor tooling robustness.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation