Auto-select sole Unity instance via PluginHub/stdio and add tests #118
Conversation
…y format
- Fix add_component to check top-level componentProperties when using
componentsToAdd as string array (e.g., ["Rigidbody"])
- Previously only worked with componentName or object array formats
- Makes API more consistent with modify action
- Add comprehensive unit tests for all three parameter formats
This allows the intuitive pattern:
{
"componentsToAdd": ["Rigidbody"],
"componentProperties": {"Rigidbody": {"mass": 5.0}}
}
…tion handling ### Motivation - Avoid probing `stdio` when the server transport is `http` and prefer PluginHub session resolution instead. - Prevent transient errors from clearing or flipping the active instance by making auto-selection defensive. - Reduce noisy exception handling by catching expected errors explicitly and re-raising system-level interrupts. - Improve observability by logging successful auto-selection at `info` and probe failures at `debug`. ### Description - Added `async def _maybe_autoselect_instance(self, ctx)` which calls `transport.unity_transport._current_transport()` and tries PluginHub `get_sessions()` first, falling back to stdio discovery only when `transport != "http"`. - Limited stdio discovery to `get_unity_connection_pool().discover_all_instances(force_refresh=True)` and only auto-select when exactly one instance is found, and set the session via `set_active_instance`. - Replaced broad `except Exception:` catches with targeted exception tuples and a final handler that re-raises `SystemExit`/`KeyboardInterrupt`, and added clearer `info`/`debug` logging messages. - Updated `_inject_unity_instance` to call `_maybe_autoselect_instance` when there is no `active_instance` and to validate PluginHub session resolution defensively. ### Testing - No automated tests were executed as part of this rollout.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughFixes property scoping in component-add logic and enhances material creation with color parsing and safe temporary-material handling. Adds middleware auto-selection of a single Unity instance (PluginHub then stdio fallback), test stubs for Starlette, and integration + unit tests validating autoselect and component-add behaviors. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,230,255,0.3)
participant Ctx as Middleware Context
participant MW as UnityInstanceMiddleware
end
rect rgba(220,255,220,0.3)
participant PH as PluginHub
participant CP as UnityConnectionPool
end
Ctx->>MW: Request injection (no active instance)
MW->>MW: _maybe_autoselect_instance(ctx)
alt PluginHub configured (HTTP transport)
MW->>PH: get_sessions()
PH-->>MW: sessions list
MW->>MW: aggregate project@hash & validate single unique
MW->>Ctx: set active instance (project@hash)
note right of MW: Selected via PluginHub
else Fallback (non-HTTP / stdio)
MW->>CP: discover instances
CP-->>MW: discovered instances
MW->>MW: validate sole instance
MW->>Ctx: set active instance (discovered id)
note right of MW: Selected via stdio
else No valid single instance
MW-->>Ctx: return None (no change)
note right of MW: No autoselect
end
MW->>Ctx: continue _inject_unity_instance() with active instance (if any)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Greptile SummaryAdds auto-selection of single Unity instances and refines exception handling to prefer PluginHub for HTTP transport and fall back to stdio discovery. The middleware now calls Major changes:
Critical issues found:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Middleware as UnityInstanceMiddleware
participant PluginHub
participant StdioPool as Unity Connection Pool
participant Context
Client->>Middleware: on_call_tool / on_read_resource
Middleware->>Middleware: _inject_unity_instance(context)
Middleware->>Middleware: get_active_instance(ctx)
alt No active instance set
Middleware->>Middleware: _maybe_autoselect_instance(ctx)
alt PluginHub.is_configured()
Middleware->>PluginHub: get_sessions()
PluginHub-->>Middleware: sessions_data
alt Exactly one session
Middleware->>Middleware: set_active_instance(ctx, project@hash)
Middleware-->>Middleware: return project@hash
else Zero or multiple sessions
Middleware-->>Middleware: continue to stdio fallback
end
end
alt transport != "http"
Middleware->>StdioPool: discover_all_instances(force_refresh=True)
StdioPool-->>Middleware: instances list
alt Exactly one instance
Middleware->>Middleware: set_active_instance(ctx, instance_id)
Middleware-->>Middleware: return instance_id
else Zero or multiple instances
Middleware-->>Middleware: return None
end
end
end
alt active_instance exists and PluginHub.is_configured()
Middleware->>PluginHub: _resolve_session_id(active_instance)
PluginHub-->>Middleware: session_id
Middleware->>Context: set_state("unity_session_id", session_id)
end
Middleware->>Context: set_state("unity_instance", active_instance)
Middleware->>Client: proceed with call_next(context)
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (1)
-
Server/src/transport/unity_instance_middleware.py, line 188 (link)logic:
RuntimeErrornot caught but_resolve_session_idraises it (e.g., "Plugin registry not configured", "Multiple Unity instances are connected"). This causes unexpected errors to bubble up.
3 files reviewed, 5 comments
| chosen, | ||
| ) | ||
| return chosen | ||
| except (ConnectionError, ValueError, KeyError, TimeoutError, AttributeError) as exc: |
There was a problem hiding this comment.
logic: RuntimeError not caught but PluginHub.get_sessions() and PluginHub._resolve_session_id() raise it (e.g., "PluginHub not configured", "No Unity plugins are currently connected"). This causes auto-select to fail unexpectedly instead of gracefully falling back to stdio.
| except (ConnectionError, ValueError, KeyError, TimeoutError, AttributeError) as exc: | |
| except (ConnectionError, ValueError, KeyError, TimeoutError, AttributeError, RuntimeError) as exc: |
Prompt To Fix With AI
This is a comment left during a code review.
Path: Server/src/transport/unity_instance_middleware.py
Line: 110:110
Comment:
**logic:** `RuntimeError` not caught but `PluginHub.get_sessions()` and `PluginHub._resolve_session_id()` raise it (e.g., "PluginHub not configured", "No Unity plugins are currently connected"). This causes auto-select to fail unexpectedly instead of gracefully falling back to stdio.
```suggestion
except (ConnectionError, ValueError, KeyError, TimeoutError, AttributeError, RuntimeError) as exc:
```
How can I resolve this? If you propose a fix, please make it concise.| chosen, | ||
| ) | ||
| return chosen | ||
| except (ConnectionError, ValueError, KeyError, TimeoutError, AttributeError) as exc: |
There was a problem hiding this comment.
logic: RuntimeError not caught but get_unity_connection_pool() and discover_all_instances() may raise it. This causes stdio auto-select to fail unexpectedly.
| except (ConnectionError, ValueError, KeyError, TimeoutError, AttributeError) as exc: | |
| except (ConnectionError, ValueError, KeyError, TimeoutError, AttributeError, RuntimeError) as exc: |
Prompt To Fix With AI
This is a comment left during a code review.
Path: Server/src/transport/unity_instance_middleware.py
Line: 141:141
Comment:
**logic:** `RuntimeError` not caught but `get_unity_connection_pool()` and `discover_all_instances()` may raise it. This causes stdio auto-select to fail unexpectedly.
```suggestion
except (ConnectionError, ValueError, KeyError, TimeoutError, AttributeError, RuntimeError) as exc:
```
How can I resolve this? If you propose a fix, please make it concise.| async def fake_get_sessions(): | ||
| call_count["sessions"] += 1 | ||
| return SimpleNamespace( | ||
| sessions={ | ||
| "session-1": SimpleNamespace(project="Ramble", hash="deadbeef"), | ||
| } | ||
| ) |
There was a problem hiding this comment.
style: Test only validates happy path - consider adding test cases for exception scenarios (e.g., when get_sessions() raises RuntimeError).
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: Server/tests/integration/test_instance_autoselect.py
Line: 45:51
Comment:
**style:** Test only validates happy path - consider adding test cases for exception scenarios (e.g., when `get_sessions()` raises `RuntimeError`).
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| class PoolStub: | ||
| def discover_all_instances(self, force_refresh=False): | ||
| assert force_refresh is True | ||
| return [SimpleNamespace(id="UnityMCPTests@cc8756d4")] |
There was a problem hiding this comment.
style: Test only validates happy path - consider adding test cases for when discover_all_instances() fails or returns zero/multiple instances.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: Server/tests/integration/test_instance_autoselect.py
Line: 92:95
Comment:
**style:** Test only validates happy path - consider adding test cases for when `discover_all_instances()` fails or returns zero/multiple instances.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
Server/tests/integration/test_instance_autoselect.py (2)
45-51: Consider adding exception scenario tests.The test validates the happy path. Consider adding tests for when
get_sessions()raises expected exceptions (e.g.,RuntimeError,ConnectionError) to verify graceful fallback behavior.
92-95: Consider adding failure scenario tests.The test validates the happy path. Consider adding tests for when
discover_all_instances()returns zero or multiple instances to verify no auto-selection occurs.Server/src/transport/unity_instance_middleware.py (2)
110-110: AddRuntimeErrorto the exception tuple.
PluginHub.get_sessions()andPluginHub._resolve_session_id()can raiseRuntimeError(e.g., "PluginHub not configured", "No Unity plugins are currently connected"). Without catching it explicitly, auto-select fails unexpectedly instead of gracefully falling back.🔎 Proposed fix
- except (ConnectionError, ValueError, KeyError, TimeoutError, AttributeError) as exc: + except (ConnectionError, ValueError, KeyError, TimeoutError, AttributeError, RuntimeError) as exc:
141-141: AddRuntimeErrorto the exception tuple.
get_unity_connection_pool()anddiscover_all_instances()may raiseRuntimeError. This causes stdio auto-select to fail unexpectedly.🔎 Proposed fix
- except (ConnectionError, ValueError, KeyError, TimeoutError, AttributeError) as exc: + except (ConnectionError, ValueError, KeyError, TimeoutError, AttributeError, RuntimeError) as exc:
🧹 Nitpick comments (2)
MCPForUnity/Editor/Tools/ManageMaterial.cs (1)
544-556: Color application silently skips when specified property doesn't exist.When
colorPropertyis specified but the material lacks that property (and also lacks_BaseColor/_Color), the color is silently not applied. This differs fromSetMaterialColor(line 192) which returns an error in this case.Consider whether this silent behavior is intentional for creation flows, or if it should warn/error for consistency.
🔎 Suggested fix to add a warning or error
if (!string.IsNullOrEmpty(colorProperty) && material.HasProperty(colorProperty)) { material.SetColor(colorProperty, color); } else if (material.HasProperty("_BaseColor")) { material.SetColor("_BaseColor", color); } else if (material.HasProperty("_Color")) { material.SetColor("_Color", color); } + else + { + // Optionally log or return error if color was requested but no suitable property found + Debug.LogWarning($"Could not find suitable color property to apply color during material creation at {materialPath}"); + }Server/tests/integration/test_instance_autoselect.py (1)
53-64: Missing stub for_resolve_session_idcauses silent error logging.After auto-selection,
_inject_unity_instancecallsPluginHub._resolve_session_id()whenis_configured()returnsTrue. Since this method isn't stubbed, it raisesAttributeErrorwhich is caught by the generic exception handler and logged as an error. The test passes but leaves noise in logs.🔎 Proposed fix - add stub for _resolve_session_id
class PluginHub: @classmethod def is_configured(cls) -> bool: return True @classmethod async def get_sessions(cls): raise AssertionError("get_sessions should be stubbed in test") + + @classmethod + async def _resolve_session_id(cls, instance_id: str): + return "session-1"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
MCPForUnity/Editor/Tools/ManageGameObject.csMCPForUnity/Editor/Tools/ManageMaterial.csServer/src/transport/unity_instance_middleware.pyServer/tests/integration/conftest.pyServer/tests/integration/test_instance_autoselect.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T05:19:59.258Z
Learnt from: dsarno
Repo: dsarno/unity-mcp PR: 85
File: tests/test_telemetry_queue_worker.py:33-37
Timestamp: 2025-10-21T05:19:59.258Z
Learning: In the unity-mcp repository, pyproject.toml is located in MCPForUnity/UnityMcpServer~/src/, not at the repository root. When testing code that imports telemetry.py, the working directory should be changed to SRC (MCPForUnity/UnityMcpServer~/src/) so that telemetry.py's get_package_version() can find pyproject.toml via relative path.
Applied to files:
Server/tests/integration/conftest.py
🧬 Code graph analysis (3)
Server/tests/integration/test_instance_autoselect.py (4)
Server/tests/integration/test_helpers.py (1)
DummyContext(16-55)Server/src/transport/unity_transport.py (1)
_current_transport(23-25)Server/src/transport/unity_instance_middleware.py (3)
_maybe_autoselect_instance(86-164)get_active_instance(74-78)_inject_unity_instance(166-213)Server/src/transport/legacy/unity_connection.py (1)
get_unity_connection_pool(652-665)
Server/src/transport/unity_instance_middleware.py (3)
Server/src/transport/unity_transport.py (1)
_current_transport(23-25)Server/tests/integration/test_instance_autoselect.py (6)
PluginHub(17-24)PluginHub(70-73)is_configured(19-20)is_configured(72-73)get_sessions(23-24)discover_all_instances(93-95)Server/src/transport/legacy/unity_connection.py (2)
get_unity_connection_pool(652-665)discover_all_instances(449-478)
MCPForUnity/Editor/Tools/ManageMaterial.cs (2)
MCPForUnity/Editor/Helpers/MaterialOps.cs (3)
Color(345-395)MaterialOps(12-396)ApplyProperties(17-155)MCPForUnity/Editor/Tools/ManageAsset.cs (1)
CreateAsset(163-277)
🪛 Ruff (0.14.10)
Server/tests/integration/test_instance_autoselect.py
24-24: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (12)
MCPForUnity/Editor/Tools/ManageGameObject.cs (1)
1453-1457: LGTM! Proper scoping fix for properties lookup.Moving the
propertiesassignment inside the string token block correctly scopes the lookup to whentypeNameis defined, preventing potential issues when the token is not a string. This maintains logical consistency: string-format tokens use top-levelcomponentProperties[typeName], while object-format tokens (line 1461) use inlineproperties.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs (1)
634-764: Excellent test coverage for the property application fix.The three new tests comprehensively validate that component properties are applied correctly across all supported input formats:
- String array format with top-level
componentProperties- Object format with inline
properties- ComponentName format with nested
componentPropertiesEach test properly verifies component addition, property values, and success indicators, with appropriate cleanup.
MCPForUnity/Editor/Tools/ManageMaterial.cs (4)
463-464: LGTM!The new parameter extraction for
colorTokenandcolorPropertyfollows the established patterns in the file and correctly handles nullable values.
505-509: Well-implemented resource cleanup pattern.The
shouldDestroyMaterialguard combined with thetry/finallyblock correctly ensures the temporaryMaterialis destroyed if asset creation fails or returns early, while avoiding double-disposal when the asset is successfully created and owned byAssetDatabase.Also applies to: 558-559, 571-577
511-530: LGTM on the color application logic.The conditional logic correctly prioritizes explicit
propertiesover thecolorparameter, preventing unintended overwrites. The check handles both specifiedcolorPropertyand fallback properties (_BaseColor,_Color) appropriately.
561-568: LGTM!Properties are correctly applied after asset creation, and the
SetDirty/SaveAssetssequence ensures changes are properly persisted. This follows the established pattern for asset creation in the codebase.Server/tests/integration/conftest.py (2)
9-11: LGTM!Adding
SERVER_SRCtosys.pathaligns with the learning that the project's source is nested underServer/src/and enables proper imports during test execution.
93-127: LGTM!The Starlette stubs appropriately isolate tests from optional dependencies. Using
sys.modules.setdefaultensures real modules aren't overwritten if already loaded.Server/src/transport/unity_instance_middleware.py (2)
86-164: Solid defensive implementation.The auto-select logic correctly prioritizes PluginHub when configured, falls back to stdio for non-http transport, and uses a defensive exception handling pattern that re-raises system interrupts while logging unexpected errors. The single-instance constraint (
len(ids) == 1) prevents accidental flipping between instances.
171-172: LGTM!Clean integration of auto-select into the existing injection flow. Only attempts auto-selection when no active instance exists, preserving explicit user selections.
Server/tests/integration/test_instance_autoselect.py (2)
9-11: LGTM!Clean helper class that wraps a context object with the expected
fastmcp_contextattribute for middleware testing.
92-95: Good assertion onforce_refresh.Validating that
force_refresh=Trueis passed ensures the test exercises the expected code path where cached instances are bypassed.
### Motivation - Prefer resolving sessions from PluginHub when HTTP transport is active to avoid probing `stdio` unnecessarily. - Auto-select the single connected Unity instance when no active instance is set to simplify developer experience. - Make auto-selection defensive to avoid transient errors clearing or flipping the active instance. - Improve observability and reduce noisy catching by tightening exception handling and making logging clearer. ### Description - Added `async def _maybe_autoselect_instance(self, ctx)` which prefers `PluginHub.get_sessions()` for HTTP transport and falls back to stdio discovery only when transport != `"http"`. - Limited stdio discovery to call `get_unity_connection_pool().discover_all_instances(force_refresh=True)` and only auto-select when exactly one instance is found, storing it via `set_active_instance`. - Refined exception handling to catch expected exception types explicitly, re-raise `SystemExit`/`KeyboardInterrupt`, and add clearer `info`/`debug` logs for probe results. - Added integration tests `Server/tests/integration/test_instance_autoselect.py` and updated `Server/tests/integration/conftest.py` to include `src` on `sys.path` and stub minimal `starlette` modules to allow isolated test runs. ### Testing - Ran `pytest tests/integration/test_instance_autoselect.py -v` which executed the new tests and reported `2 passed`. - The new tests validate PluginHub-based auto-selection and stdio fallback, and verify that PluginHub calls are not redundantly invoked. - Test suite uses `asyncio.run(...)` to execute middleware async paths and stubs `transport` submodules to isolate the unit under test. - No other automated tests were modified as part of this change.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Server/tests/integration/test_instance_autoselect.py (1)
1-225: Test coverage is solid and addresses previous feedback.The test suite validates:
- PluginHub auto-selection with single session (lines 14-68)
- stdio auto-selection with single instance (lines 71-113)
- Skipping auto-select for multiple sessions (lines 115-154)
- Skipping auto-select when no instances exist (lines 156-190)
- Error handling for PluginHub failures (lines 192-225)
This addresses the previous review comment requesting exception scenario tests.
Optional: Consider adding stdio error scenario test
The PluginHub error path is tested (lines 192-225), but there's no equivalent test for stdio discovery errors. While the production code handles these gracefully, a test would document the behavior:
def test_auto_select_handles_stdio_errors(monkeypatch): # Similar setup to test_auto_selects_single_instance_via_stdio # but PoolStub.discover_all_instances raises ConnectionError ...This is optional—the current coverage is sufficient for a "Chill" review.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
MCPForUnity/Editor/Tools/ManageMaterial.csServer/src/transport/unity_instance_middleware.pyServer/tests/integration/test_instance_autoselect.py
🧰 Additional context used
🧬 Code graph analysis (2)
MCPForUnity/Editor/Tools/ManageMaterial.cs (2)
MCPForUnity/Editor/Helpers/MaterialOps.cs (3)
Color(345-395)MaterialOps(12-396)ApplyProperties(17-155)MCPForUnity/Editor/Tools/ManageAsset.cs (1)
CreateAsset(163-277)
Server/src/transport/unity_instance_middleware.py (3)
Server/src/transport/unity_transport.py (1)
_current_transport(23-25)Server/src/services/tools/set_active_instance.py (1)
set_active_instance(15-112)Server/src/transport/legacy/unity_connection.py (2)
get_unity_connection_pool(652-665)discover_all_instances(449-478)
🪛 Ruff (0.14.10)
Server/tests/integration/test_instance_autoselect.py
24-24: Avoid specifying long messages outside the exception class
(TRY003)
27-27: Unused class method argument: instance_id
(ARG003)
125-125: Avoid specifying long messages outside the exception class
(TRY003)
202-202: Avoid specifying long messages outside the exception class
(TRY003)
218-218: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (7)
Server/src/transport/unity_instance_middleware.py (2)
86-164: Auto-selection logic is comprehensive and defensive.The implementation correctly:
- Prefers PluginHub sessions when configured, falling back to stdio discovery
- Auto-selects only when exactly one instance is found
- Includes
RuntimeErrorin exception handlers (lines 110, 141), addressing previous review feedback- Re-raises
SystemExit/KeyboardInterruptto avoid swallowing critical signals- Logs failures at debug level without propagating transient errors
The dynamic imports (lines 89, 127) are unconventional but likely intentional to avoid circular dependencies or enable lazy loading.
171-172: LGTM: Auto-select integration is clean.Calling
_maybe_autoselect_instancewhen no active instance exists is the right approach for seamless developer experience.MCPForUnity/Editor/Tools/ManageMaterial.cs (5)
463-464: LGTM: New color parameters for material creation.The extraction of optional
colorTokenandcolorPropertyparameters follows the established pattern and enables color application during material creation.
505-509: LGTM: Proper resource management pattern.The temporary material with a cleanup flag and try-finally block ensures proper resource management, preventing leaks if asset creation fails.
513-530: LGTM: Defensive color application logic.The logic correctly prevents overwriting color properties specified in the
propertiesobject, checking both explicitcolorPropertynames and standard fallback properties.
564-573: LGTM: Correct asset creation flow.The sequence of creating the asset, transferring ownership (clearing the cleanup flag), applying additional properties, and marking dirty follows Unity's asset management best practices.
577-583: LGTM: Proper cleanup ensures no resource leaks.The finally block correctly cleans up the temporary material only when asset creation didn't complete, preventing resource leaks on error paths.
| if (!string.IsNullOrEmpty(colorProperty) && material.HasProperty(colorProperty)) | ||
| { | ||
| material.SetColor(colorProperty, color); | ||
| } | ||
| else if (material.HasProperty("_BaseColor")) | ||
| { | ||
| material.SetColor("_BaseColor", color); | ||
| } | ||
| else if (material.HasProperty("_Color")) | ||
| { | ||
| material.SetColor("_Color", color); | ||
| } | ||
| else | ||
| { | ||
| Debug.LogWarning( | ||
| $"Could not find suitable color property to apply color during material creation at {materialPath}" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Consider explicit error when specified colorProperty doesn't exist.
When a user explicitly specifies colorProperty but that property doesn't exist on the material, the code silently falls back to _BaseColor or _Color. This differs from SetMaterialColor (line 192), which returns an error in similar cases.
Consider adding an explicit check and error return when the user-specified property doesn't exist:
🔎 Suggested improvement for explicit property validation
if (shouldApplyColor)
{
Color color;
try
{
color = MaterialOps.ParseColor(colorToken, ManageGameObject.InputSerializer);
}
catch (Exception e)
{
return new { status = "error", message = $"Invalid color format: {e.Message}" };
}
- if (!string.IsNullOrEmpty(colorProperty) && material.HasProperty(colorProperty))
+ if (!string.IsNullOrEmpty(colorProperty))
{
- material.SetColor(colorProperty, color);
+ if (material.HasProperty(colorProperty))
+ {
+ material.SetColor(colorProperty, color);
+ }
+ else
+ {
+ return new { status = "error", message = $"Specified color property '{colorProperty}' does not exist on shader {shaderName}" };
+ }
}
else if (material.HasProperty("_BaseColor"))
{
material.SetColor("_BaseColor", color);
}
else if (material.HasProperty("_Color"))
{
material.SetColor("_Color", color);
}
else
{
- Debug.LogWarning(
- $"Could not find suitable color property to apply color during material creation at {materialPath}"
- );
+ return new { status = "error", message = "Could not find suitable color property (_BaseColor or _Color) on shader" };
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
MCPForUnity/Editor/Tools/ManageMaterial.cs around lines 544-561: the current
logic falls back to _BaseColor/_Color when a user-specified colorProperty is
provided but missing on the material; change it to explicitly treat a missing
user-specified property as an error (match SetMaterialColor at ~line 192).
Concretely, if colorProperty is not null/empty and
material.HasProperty(colorProperty) is false, log an error (or return a failure)
indicating the specified property doesn't exist on the material and abort
applying the color; only allow fallback to _BaseColor/_Color when no explicit
colorProperty was provided. Ensure the error path returns the same error/false
contract used by SetMaterialColor.
dsarno
deleted the
codex/clarify-issue-with-custom-tools-in-repo-rrcsjq
branch
Motivation
stdiounnecessarily.Description
async def _maybe_autoselect_instance(self, ctx)which prefersPluginHub.get_sessions()for HTTP transport and falls back to stdio discovery only when transport !="http".get_unity_connection_pool().discover_all_instances(force_refresh=True)and only auto-select when exactly one instance is found, storing it viaset_active_instance.SystemExit/KeyboardInterrupt, and add clearerinfo/debuglogs for probe results.Server/tests/integration/test_instance_autoselect.pyand updatedServer/tests/integration/conftest.pyto includesrconsys.pathand stub minimalstarlettemodules to allow isolated test runs.Testing
pytest tests/integration/test_instance_autoselect.py -vwhich executed the new tests and reported2 passed.asyncio.run(...)to execute middleware async paths and stubstransportsubmodules to isolate the unit under test.Codex Task
Summary by CodeRabbit
Bug Fixes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.