fix: enforce tool toggle checks in batch_execute#741
Conversation
batch_execute was bypassing disabled-tool checks by calling CommandRegistry.InvokeCommandAsync directly. Add the same IsToolEnabled guard that TransportCommandDispatcher uses. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds a disabled-tool guard to batch_execute so batch tool invocations respect tool enablement and surface a consistent error when blocked. Sequence diagram for batch_execute tool invocation with disabled-tool guardsequenceDiagram
participant BatchExecute
participant ToolDiscovery
participant CommandRegistry
loop For_each_tool_in_batch
BatchExecute->>ToolDiscovery: GetToolMetadata(toolName)
alt ToolMetadata_exists
BatchExecute->>ToolDiscovery: IsToolEnabled(toolName)
alt Tool_disabled
BatchExecute->>BatchExecute: Increment_invocationFailureCount
BatchExecute->>BatchExecute: Set_anyCommandFailed_true
BatchExecute->>BatchExecute: Add_error_result_with_ErrorResponse
alt failFast_true
BatchExecute->>BatchExecute: Break_loop
else failFast_false
BatchExecute->>BatchExecute: Continue_to_next_tool
end
else Tool_enabled
BatchExecute->>CommandRegistry: InvokeCommandAsync(toolName, commandParams)
CommandRegistry-->>BatchExecute: result
BatchExecute->>BatchExecute: Add_success_result
end
else ToolMetadata_missing
BatchExecute->>BatchExecute: Handle_missing_tool_metadata
end
end
Updated class diagram for BatchExecute disabled-tool guard integrationclassDiagram
class BatchExecute {
+Task~object~ HandleCommand(JObject params)
-int invocationFailureCount
-bool anyCommandFailed
-bool failFast
}
class MCPServiceLocator {
+ToolDiscoveryService ToolDiscovery
}
class ToolDiscoveryService {
+ToolMetadata GetToolMetadata(string toolName)
+bool IsToolEnabled(string toolName)
}
class ToolMetadata {
+string Name
+string Description
+bool Enabled
}
class CommandRegistry {
+Task~object~ InvokeCommandAsync(string toolName, JObject commandParams)
}
class ErrorResponse {
+ErrorResponse(string message)
+string Message
}
BatchExecute ..> MCPServiceLocator : uses
MCPServiceLocator *-- ToolDiscoveryService : provides
ToolDiscoveryService --> ToolMetadata : returns
BatchExecute ..> ToolDiscoveryService : queries
BatchExecute ..> CommandRegistry : invokes
BatchExecute ..> ErrorResponse : creates
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a pre-invocation validation check to BatchExecute that blocks command execution for disabled tools. When a disabled tool is detected, the command is marked as failed, failure counters increment, and the loop continues or breaks based on the failFast setting. A new import for MCPForUnity.Editor.Services enables tool state querying. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
✨ 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 |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new disabled-tool guard duplicates logic from
TransportCommandDispatcher; consider extracting a shared helper or extension so the behavior and error messaging stay in sync in both call paths. - The
toolMetalocal is only used for a null check before callingIsToolEnabled; ifIsToolEnabledalready handles unknown tools safely, you can simplify by calling it directly without fetching metadata.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new disabled-tool guard duplicates logic from `TransportCommandDispatcher`; consider extracting a shared helper or extension so the behavior and error messaging stay in sync in both call paths.
- The `toolMeta` local is only used for a null check before calling `IsToolEnabled`; if `IsToolEnabled` already handles unknown tools safely, you can simplify by calling it directly without fetching metadata.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
batch_execute was bypassing disabled-tool checks by calling CommandRegistry.InvokeCommandAsync directly. Add the same IsToolEnabled guard that TransportCommandDispatcher uses. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
batch_execute was bypassing disabled-tool checks by calling CommandRegistry.InvokeCommandAsync directly. Add the same IsToolEnabled guard that TransportCommandDispatcher uses. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
batch_executewas bypassing disabled-tool checks by callingCommandRegistry.InvokeCommandAsyncdirectlyIsToolEnabledguard thatTransportCommandDispatcheralready uses"Tool 'X' is disabled in the Unity Editor."errors in batch resultsTest plan
🤖 Generated with Claude Code
Summary by Sourcery
Bug Fixes:
Summary by CodeRabbit