Conversation
…ndling - Block execution of disabled resources in TransportCommandDispatcher with clear error message - Add parse_resource_response() utility to handle error responses without Pydantic validation failures - Replace inline response parsing with parse_resource_response() across all resource handlers - Export parse_resource_response from models/__init__.py for consistent usage
…her with clear error message Add tool enable/disable enforcement before command execution. Check tool metadata and enabled state, returning error response if tool is disabled. Prevents execution of disabled tools with user-friendly error message.
Contributor
Reviewer's GuideAdds a new Resources tab to the MCP for Unity editor mirroring the Tools tab, introduces a reflection-based resource discovery service with enable/disable persistence, and ensures both tools and resources are blocked at dispatch-time when disabled, while tightening Python server-side resource response parsing to gracefully handle error payloads. Sequence diagram for dispatch-time blocking of disabled tools and resourcessequenceDiagram
participant LLM
participant Server
participant UnityEditor
participant TransportCommandDispatcher as TransportDispatcher
participant MCPServiceLocator as ServiceLocator
participant ResourceDiscovery as ResourceDiscoveryService
participant ToolDiscovery as ToolDiscoveryService
LLM->>Server: send MCP command
Server->>UnityEditor: MCP command request
UnityEditor->>TransportDispatcher: ProcessCommand(command)
TransportDispatcher->>ServiceLocator: ResourceDiscovery
ServiceLocator-->>TransportDispatcher: IResourceDiscoveryService
TransportDispatcher->>ResourceDiscovery: GetResourceMetadata(command.type)
ResourceDiscovery-->>TransportDispatcher: ResourceMetadata or null
alt resource metadata exists and disabled
TransportDispatcher->>ResourceDiscovery: IsResourceEnabled(command.type)
ResourceDiscovery-->>TransportDispatcher: false
TransportDispatcher-->>Server: SerializeError("Resource disabled")
Server-->>LLM: error response (resource disabled)
else no resource metadata or enabled
TransportDispatcher->>ServiceLocator: ToolDiscovery
ServiceLocator-->>TransportDispatcher: IToolDiscoveryService
TransportDispatcher->>ToolDiscovery: GetToolMetadata(command.type)
ToolDiscovery-->>TransportDispatcher: ToolMetadata or null
alt tool metadata exists and disabled
TransportDispatcher->>ToolDiscovery: IsToolEnabled(command.type)
ToolDiscovery-->>TransportDispatcher: false
TransportDispatcher-->>Server: SerializeError("Tool disabled")
Server-->>LLM: error response (tool disabled)
else no tool metadata or enabled
TransportDispatcher->>TransportDispatcher: CommandRegistry.ExecuteCommand()
TransportDispatcher-->>Server: command result
Server-->>LLM: success response
end
end
Sequence diagram for Python resource service parsing Unity responsessequenceDiagram
participant Client as LLMClient
participant ResourceService as PythonResourceService
participant UnityBridge as UnityConnection
participant UnityEditor
participant ParseResource as parse_resource_response
participant MCPModel as MCPResponseSubclass
Client->>ResourceService: call get_layers or other resource
ResourceService->>UnityBridge: send Unity command
UnityBridge->>UnityEditor: execute resource command
UnityEditor-->>UnityBridge: response dict or error shape
UnityBridge-->>ResourceService: raw response
ResourceService->>ParseResource: parse_resource_response(response, TypedResponse)
alt response is error dict
ParseResource-->>ResourceService: MCPResponse(success=False, error, message)
else response is success dict
ParseResource-->>ResourceService: TypedResponse(**response)
end
ResourceService-->>Client: MCPResponse or TypedResponse
Class diagram for new resource discovery and editor integrationclassDiagram
class MCPForUnityEditorWindow {
- McpResourcesSection resourcesSection
- ToolbarToggle resourcesTabToggle
- VisualElement resourcesPanel
- bool resourcesLoaded
+ void CreateGUI()
- void EnsureResourcesLoaded()
- void SetupTabs()
- void SwitchPanel(ActivePanel panel)
}
class McpResourcesSection {
- Dictionary~string, Toggle~ resourceToggleMap
- Label summaryLabel
- Label noteLabel
- Button enableAllButton
- Button disableAllButton
- Button rescanButton
- VisualElement categoryContainer
- List~ResourceMetadata~ allResources
+ VisualElement Root
+ McpResourcesSection(VisualElement root)
- void CacheUIElements()
- void RegisterCallbacks()
+ void Refresh()
- void BuildCategory(string title, string prefsSuffix, IEnumerable~ResourceMetadata~ resources)
- VisualElement CreateResourceRow(ResourceMetadata resource)
- void HandleToggleChange(ResourceMetadata resource, bool enabled, bool updateSummary)
- void SetAllResourcesState(bool enabled)
- void UpdateSummary()
- void AddInfoLabel(string message)
- static Label CreateTag(string text)
}
class IResourceDiscoveryService {
<<interface>>
+ List~ResourceMetadata~ DiscoverAllResources()
+ ResourceMetadata GetResourceMetadata(string resourceName)
+ List~ResourceMetadata~ GetEnabledResources()
+ bool IsResourceEnabled(string resourceName)
+ void SetResourceEnabled(string resourceName, bool enabled)
+ void InvalidateCache()
}
class ResourceDiscoveryService {
- Dictionary~string, ResourceMetadata~ _cachedResources
+ List~ResourceMetadata~ DiscoverAllResources()
+ ResourceMetadata GetResourceMetadata(string resourceName)
+ List~ResourceMetadata~ GetEnabledResources()
+ bool IsResourceEnabled(string resourceName)
+ void SetResourceEnabled(string resourceName, bool enabled)
+ void InvalidateCache()
- ResourceMetadata ExtractResourceMetadata(Type type, McpForUnityResourceAttribute resourceAttr)
- void EnsurePreferenceInitialized(ResourceMetadata metadata)
- static string GetResourcePreferenceKey(string resourceName)
}
class ResourceMetadata {
+ string Name
+ string Description
+ string ClassName
+ string Namespace
+ string AssemblyName
+ bool IsBuiltIn
}
class MCPServiceLocator {
- IResourceDiscoveryService _resourceDiscoveryService
+ IResourceDiscoveryService ResourceDiscovery
+ void Register~T~(T implementation)
+ void Reset()
}
class McpForUnityResourceAttribute {
+ string ResourceName
+ string Description
}
class StringCaseUtility {
<<static>>
+ bool IsBuiltInMcpType(Type type, string assemblyName, string builtInNamespacePrefix)
+ string ToSnakeCase(string input)
}
class ToolDiscoveryService {
+ List~ToolMetadata~ DiscoverAllTools()
- ToolMetadata ExtractToolMetadata(Type type, McpForUnityToolAttribute toolAttr)
}
MCPForUnityEditorWindow --> McpResourcesSection : owns
MCPForUnityEditorWindow --> MCPServiceLocator : uses
McpResourcesSection --> MCPServiceLocator : uses
MCPServiceLocator --> IResourceDiscoveryService : exposes
ResourceDiscoveryService ..|> IResourceDiscoveryService
ResourceDiscoveryService --> ResourceMetadata : creates
ResourceDiscoveryService --> StringCaseUtility : uses
ResourceDiscoveryService --> McpForUnityResourceAttribute : reads
McpResourcesSection --> ResourceMetadata : displays
ToolDiscoveryService --> StringCaseUtility : uses
McpForUnityResourceAttribute <.. ResourceMetadata
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughAdds resource discovery and metadata types, a ResourceDiscovery service registered in MCPServiceLocator, EditorPrefs-backed enable/disable toggles, a Resources editor UI/tab with foldouts and toggles, pre-execution guards for disabled resources/tools, and centralized server-side resource response parsing. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
actor User as Editor User
participant Window as MCPForUnityEditorWindow
participant ResourceSection as McpResourcesSection
participant Locator as MCPServiceLocator
participant Discovery as ResourceDiscoveryService
participant Reflection as TypeCache/Reflection
participant EditorPrefs as EditorPrefs
User->>Window: Click Resources tab
Window->>Window: EnsureResourcesLoaded()
Window->>ResourceSection: Create/Refresh UI
ResourceSection->>Locator: ResourceDiscovery.DiscoverAllResources()
Locator->>Discovery: DiscoverAllResources()
Discovery->>Reflection: Scan types for MCPForUnityResourceAttribute
Reflection-->>Discovery: Types list
loop each discovered type
Discovery->>Discovery: ExtractResourceMetadata(type, attribute)
Discovery->>EditorPrefs: EnsurePreferenceInitialized(resourceName)
end
Discovery-->>Locator: List<ResourceMetadata>
Locator-->>ResourceSection: List<ResourceMetadata>
ResourceSection->>ResourceSection: Build UI with toggles/foldouts
User->>ResourceSection: Toggle resource
ResourceSection->>Discovery: SetResourceEnabled(name, enabled)
Discovery->>EditorPrefs: Persist enabled state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
parse_resource_response, consider always returning anMCPResponse(e.g., wrapping non-dict responses as an error) so the behavior matches the return type annotation and callers don’t have to handle raw values. - In
ResourceDiscoveryService, resources are keyed solely byresourceName, so duplicate names will silently override each other; it may be safer to detect and log a warning or include namespace/type in the key to avoid accidental collisions. - The built-in type detection in
StringCaseUtility.IsBuiltInMcpTypehardcodes the assembly name string; consider deriving this from a known type (e.g.,typeof(SomeEditorType).Assembly.GetName().Name) or centralizing the constant to avoid drift if the assembly name changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `parse_resource_response`, consider always returning an `MCPResponse` (e.g., wrapping non-dict responses as an error) so the behavior matches the return type annotation and callers don’t have to handle raw values.
- In `ResourceDiscoveryService`, resources are keyed solely by `resourceName`, so duplicate names will silently override each other; it may be safer to detect and log a warning or include namespace/type in the key to avoid accidental collisions.
- The built-in type detection in `StringCaseUtility.IsBuiltInMcpType` hardcodes the assembly name string; consider deriving this from a known type (e.g., `typeof(SomeEditorType).Assembly.GetName().Name`) or centralizing the constant to avoid drift if the assembly name changes.
## Individual Comments
### Comment 1
<location> `MCPForUnity/Editor/Services/ResourceDiscoveryService.cs:23-32` </location>
<code_context>
+ _cachedResources = new Dictionary<string, ResourceMetadata>();
</code_context>
<issue_to_address>
**issue (bug_risk):** Handle duplicate resource names so later discoveries don’t silently overwrite earlier ones.
Keying `Dictionary<string, ResourceMetadata>` by `metadata.Name` will silently overwrite colliding resource names; add collision detection with logging or explicit precedence to surface misconfigurations.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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: 2
🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/Services/ResourceDiscoveryService.cs`:
- Around line 56-64: GetResourceMetadata can pass a null/empty key to
_cachedResources.TryGetValue which throws; add a defensive guard at the start of
GetResourceMetadata to return null if resourceName is null or empty, ensure
_cachedResources is populated by calling DiscoverAllResources() as currently
done, and then safely use _cachedResources.TryGetValue(resourceName, out var
metadata) to return metadata or null; reference GetResourceMetadata,
_cachedResources, and DiscoverAllResources.
- Around line 44-49: When discovering resources, guard against duplicate
resource names so earlier entries aren't silently overwritten: after calling
ExtractResourceMetadata(type, resourceAttr) and before assigning to
_cachedResources[metadata.Name], check whether _cachedResources already contains
metadata.Name; if it does, log a warning (including the conflicting type names
and metadata.Name) and skip adding the new metadata (or alternatively
throw/require explicit naming), otherwise add and call
EnsurePreferenceInitialized(metadata); reference the ExtractResourceMetadata
call, the _cachedResources dictionary, metadata.Name, and
EnsurePreferenceInitialized to locate and implement the guard.
🧹 Nitpick comments (2)
Server/src/models/unity_response.py (1)
52-70: Consider handling non-dict responses more explicitly.When
responseis not a dict (lines 59-60), the function returns it directly. However, the return type annotation isMCPResponse, which could cause type confusion if the caller receives something unexpected (e.g.,None, a string, or another primitive).This may be intentional as a pass-through for already-parsed responses, but if so, the return type should be
Anyor the docstring should clarify this behavior.💡 Option: Return an error MCPResponse for unexpected input types
def parse_resource_response(response: Any, typed_cls: Type[MCPResponse]) -> MCPResponse: """Parse a Unity response into a typed response class. Returns a base ``MCPResponse`` for error responses so that typed subclasses with strict ``data`` fields (e.g. ``list[str]``) don't raise Pydantic validation errors when ``data`` is ``None``. """ if not isinstance(response, dict): - return response + return MCPResponse( + success=False, + error=f"Expected dict response, got {type(response).__name__}", + ) # Detect errors from both normalized (success=False) and raw (status="error") shapes. if response.get("success") is False or response.get("status") == "error":MCPForUnity/Editor/Windows/Components/Resources/McpResourcesSection.cs (1)
18-81: Consider centralizing required UI element validation to avoid silent no-ops.
The current null-conditional checks can mask UXML mismatches, leaving the Resources tab empty without a clear error. A single “UI ready” validation keeps the flow fail-fast and avoids repeated null checks.♻️ Proposed refactor (centralize validation and short-circuit)
@@ - private List<ResourceMetadata> allResources = new(); + private List<ResourceMetadata> allResources = new(); + private bool uiReady; @@ private void CacheUIElements() { summaryLabel = Root.Q<Label>("resources-summary"); noteLabel = Root.Q<Label>("resources-note"); enableAllButton = Root.Q<Button>("enable-all-resources-button"); disableAllButton = Root.Q<Button>("disable-all-resources-button"); rescanButton = Root.Q<Button>("rescan-resources-button"); categoryContainer = Root.Q<VisualElement>("resource-category-container"); + + uiReady = summaryLabel != null && noteLabel != null + && enableAllButton != null && disableAllButton != null + && rescanButton != null && categoryContainer != null; + if (!uiReady) + { + McpLog.Error("Resources UXML is missing required elements."); + } } @@ private void RegisterCallbacks() { - if (enableAllButton != null) - { - enableAllButton.AddToClassList("tool-action-button"); - enableAllButton.style.marginRight = 4; - enableAllButton.clicked += () => SetAllResourcesState(true); - } - - if (disableAllButton != null) - { - disableAllButton.AddToClassList("tool-action-button"); - disableAllButton.style.marginRight = 4; - disableAllButton.clicked += () => SetAllResourcesState(false); - } - - if (rescanButton != null) - { - rescanButton.AddToClassList("tool-action-button"); - rescanButton.clicked += () => - { - McpLog.Info("Rescanning MCP resources from the editor window."); - MCPServiceLocator.ResourceDiscovery.InvalidateCache(); - Refresh(); - }; - } + if (!uiReady) + { + return; + } + + enableAllButton.AddToClassList("tool-action-button"); + enableAllButton.style.marginRight = 4; + enableAllButton.clicked += () => SetAllResourcesState(true); + + disableAllButton.AddToClassList("tool-action-button"); + disableAllButton.style.marginRight = 4; + disableAllButton.clicked += () => SetAllResourcesState(false); + + rescanButton.AddToClassList("tool-action-button"); + rescanButton.clicked += () => + { + McpLog.Info("Rescanning MCP resources from the editor window."); + MCPServiceLocator.ResourceDiscovery.InvalidateCache(); + Refresh(); + }; } @@ public void Refresh() { + if (!uiReady) + { + return; + } resourceToggleMap.Clear(); - categoryContainer?.Clear(); + categoryContainer.Clear();Based on learnings: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Member
Author
|
This might be the least amount of comments I've received from Sourcery and CodeRabbit. My goodness does this feel good! |
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
1 participant
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.
Description
Shows the resources in the tab & fixes the enable/disable function for built-in tools
Type of Change
Changes Made
Add a tab for displaying resources, similar to how we had a tab for displaying tools. Also, when a user disables a tool or resource, we return an error to the LLM letting them know the tool was disabled and they shouldn't use it.
Testing/Screenshots/Recordings
Documentation Updates
tools/UPDATE_DOCS_PROMPT.md(recommended)tools/UPDATE_DOCS.mdRelated Issues
Closes #648
Additional Notes
Summary by Sourcery
Add resource management to the MCP for Unity editor and ensure disabled tools and resources are respected across the Unity editor and server.
New Features:
Bug Fixes:
Enhancements:
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
✏️ Tip: You can customize this high-level summary in your review settings.