feat: Add tool annotations for improved LLM tool understanding #480
Conversation
Reviewer's GuideAdds MCP ToolAnnotations (title, readOnlyHint, destructiveHint) across all Unity MCP tools to improve client understanding of tool behavior and safety characteristics, plus a uv.lock update for the newer MCP dependency. Sequence diagram for LLM client using tool annotations for safety decisionssequenceDiagram
actor User
participant LlmClient
participant McpServer
participant UnityEditor
User->>LlmClient: Request Unity project change
LlmClient->>McpServer: List available tools
McpServer-->>LlmClient: Tool metadata with ToolAnnotations
LlmClient->>LlmClient: Classify tools by readOnlyHint and destructiveHint
LlmClient->>User: Present plan using read-only tools first
User->>LlmClient: Approve read-only steps
LlmClient->>McpServer: Call FindInFileTool (readOnlyHint true)
McpServer->>UnityEditor: Read file contents
UnityEditor-->>McpServer: File matches
McpServer-->>LlmClient: Read-only results
LlmClient->>LlmClient: Auto-approve further read-only tools
LlmClient->>User: Request confirmation for destructive tools
User->>LlmClient: Confirm destructive actions
LlmClient->>McpServer: Call ApplyTextEditsTool (destructiveHint true)
McpServer->>UnityEditor: Apply code edits
UnityEditor-->>McpServer: Edit success
McpServer-->>LlmClient: Operation result
LlmClient-->>User: Summarize changes and status
Class diagram for MCP tools using ToolAnnotationsclassDiagram
class ToolAnnotations {
+string title
+bool readOnlyHint
+bool destructiveHint
}
class ApplyTextEditsTool {
+Context ctx
+string uri
+string edits
+ToolAnnotations annotations
}
class CreateScriptTool {
+Context ctx
+string path
+string contents
+ToolAnnotations annotations
}
class DeleteScriptTool {
+Context ctx
+string uri
+ToolAnnotations annotations
}
class ValidateScriptTool {
+Context ctx
+string uri
+ToolAnnotations annotations
}
class ManageScriptTool {
+Context ctx
+string action
+ToolAnnotations annotations
}
class ManageScriptCapabilitiesTool {
+Context ctx
+ToolAnnotations annotations
}
class GetShaTool {
+Context ctx
+string uri
+ToolAnnotations annotations
}
class DebugRequestContextTool {
+Context ctx
+ToolAnnotations annotations
}
class FindInFileTool {
+Context ctx
+string uri
+string pattern
+ToolAnnotations annotations
}
class ExecuteMenuItemTool {
+Context ctx
+string menu_path
+ToolAnnotations annotations
}
class ManageAssetTool {
+Context ctx
+string action
+ToolAnnotations annotations
}
class ManageEditorTool {
+Context ctx
+string action
+ToolAnnotations annotations
}
class ManageGameobjectTool {
+Context ctx
+string action
+ToolAnnotations annotations
}
class ManageMaterialTool {
+Context ctx
+string action
+ToolAnnotations annotations
}
class ManagePrefabsTool {
+Context ctx
+string action
+ToolAnnotations annotations
}
class ManageSceneTool {
+Context ctx
+string action
+ToolAnnotations annotations
}
class ManageShaderTool {
+Context ctx
+string action
+ToolAnnotations annotations
}
class ReadConsoleTool {
+Context ctx
+string action
+ToolAnnotations annotations
}
class SetActiveInstanceTool {
+Context ctx
+string instance
+ToolAnnotations annotations
}
class BatchExecuteTool {
+Context ctx
+list batch
+ToolAnnotations annotations
}
class ExecuteCustomToolTool {
+Context ctx
+string tool_name
+dict parameters
+ToolAnnotations annotations
}
class RunTestsTool {
+Context ctx
+string filter
+ToolAnnotations annotations
}
class ScriptApplyEditsTool {
+Context ctx
+string name
+list edits
+ToolAnnotations annotations
}
ToolAnnotations <.. ApplyTextEditsTool : uses
ToolAnnotations <.. CreateScriptTool : uses
ToolAnnotations <.. DeleteScriptTool : uses
ToolAnnotations <.. ValidateScriptTool : uses
ToolAnnotations <.. ManageScriptTool : uses
ToolAnnotations <.. ManageScriptCapabilitiesTool : uses
ToolAnnotations <.. GetShaTool : uses
ToolAnnotations <.. DebugRequestContextTool : uses
ToolAnnotations <.. FindInFileTool : uses
ToolAnnotations <.. ExecuteMenuItemTool : uses
ToolAnnotations <.. ManageAssetTool : uses
ToolAnnotations <.. ManageEditorTool : uses
ToolAnnotations <.. ManageGameobjectTool : uses
ToolAnnotations <.. ManageMaterialTool : uses
ToolAnnotations <.. ManagePrefabsTool : uses
ToolAnnotations <.. ManageSceneTool : uses
ToolAnnotations <.. ManageShaderTool : uses
ToolAnnotations <.. ReadConsoleTool : uses
ToolAnnotations <.. SetActiveInstanceTool : uses
ToolAnnotations <.. BatchExecuteTool : uses
ToolAnnotations <.. ExecuteCustomToolTool : uses
ToolAnnotations <.. RunTestsTool : uses
ToolAnnotations <.. ScriptApplyEditsTool : uses
class McpForUnityToolDecorator {
+ToolAnnotations annotations
+string description
+string name
}
McpForUnityToolDecorator o-- ToolAnnotations
McpForUnityToolDecorator <.. ApplyTextEditsTool
McpForUnityToolDecorator <.. ValidateScriptTool
McpForUnityToolDecorator <.. DebugRequestContextTool
McpForUnityToolDecorator <.. BatchExecuteTool
McpForUnityToolDecorator <.. ScriptApplyEditsTool
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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. 📝 WalkthroughWalkthroughThis PR adds ToolAnnotations metadata to many Changes
Sequence Diagram(s)(Skipped — changes are metadata and signature additions without new multi-component control flow.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (20)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (9)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-10-03T22:11:46.002ZApplied to files:
📚 Learning: 2025-11-05T18:23:12.349ZApplied to files:
📚 Learning: 2025-12-29T04:54:25.306ZApplied to files:
🧬 Code graph analysis (1)Server/src/services/tools/manage_shader.py (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (11)
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}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Several tools that can be either read-only or mutating depending on the action (e.g.,
manage_*routers,read_console,batch_execute) are marked unconditionally withdestructiveHint=True; consider whether you want more granular signaling (e.g., per-action parameters or separate tools) so clients can auto-approve genuinely read-only invocations. - The
ToolAnnotationsblocks repeat very similartitle/readOnlyHint/destructiveHintdefinitions across many tools; consider centralizing these (e.g., constants or a small helper wrapper aroundmcp_for_unity_tool) to reduce duplication and make future changes to annotation conventions easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several tools that can be either read-only or mutating depending on the action (e.g., `manage_*` routers, `read_console`, `batch_execute`) are marked unconditionally with `destructiveHint=True`; consider whether you want more granular signaling (e.g., per-action parameters or separate tools) so clients can auto-approve genuinely read-only invocations.
- The `ToolAnnotations` blocks repeat very similar `title`/`readOnlyHint`/`destructiveHint` definitions across many tools; consider centralizing these (e.g., constants or a small helper wrapper around `mcp_for_unity_tool`) to reduce duplication and make future changes to annotation conventions easier.
## Individual Comments
### Comment 1
<location> `Server/src/services/tools/batch_execute.py:25` </location>
<code_context>
from transport.plugin_hub import PluginHub
@mcp_for_unity_tool(
- description="Return the current FastMCP request context details (client_id, session_id, and meta dump)."
+ description="Return the current FastMCP request context details (client_id, session_id, and meta dump).",
</code_context>
<issue_to_address>
**suggestion:** Clarify the destructive nature of `batch_execute`, since it can also wrap read-only calls.
Labeling `batch_execute` as destructive is reasonable for safety, but it will cause all uses to be treated as risky, even read-only batches. Consider adjusting the description to clarify that *batches may be destructive depending on the tools they contain*, so clients and agents can reflect that nuance instead of assuming every `batch_execute` call mutates state.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Thank you for the review feedback! Re: Dual-purpose tools: I've updated the descriptions for I've kept Re: Centralization: Valid point for larger codebases. For this PR's scope (23 tools), I prefer keeping annotations explicit per-tool for clarity and auditability. The repetitive pattern is intentional—it makes each tool self-documenting and easy to audit. Happy to refactor if the maintainers prefer centralized constants. Changes pushed in commit 79c5ec7. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Server/src/services/tools/manage_script.py (1)
512-519: Consolidate multiple annotation strings into single descriptions for consistency.Lines 513-519 contain multiple string metadata values in
Annotatedtypes (name,path,contents,script_type). The codebase convention uses single description strings. Combine the metadata into a single, more informative description string—for example:name: Annotated[str, "Script name without .cs extension"]andpath: Annotated[str, "Path under Assets/ to create the script, e.g., 'Assets/Scripts/My.cs'"].
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Server/src/services/tools/batch_execute.pyServer/src/services/tools/manage_asset.pyServer/src/services/tools/manage_editor.pyServer/src/services/tools/manage_gameobject.pyServer/src/services/tools/manage_material.pyServer/src/services/tools/manage_scene.pyServer/src/services/tools/manage_script.pyServer/src/services/tools/manage_shader.pyServer/src/services/tools/read_console.py
🚧 Files skipped from review as they are similar to previous changes (5)
- Server/src/services/tools/batch_execute.py
- Server/src/services/tools/manage_shader.py
- Server/src/services/tools/manage_asset.py
- Server/src/services/tools/manage_editor.py
- Server/src/services/tools/read_console.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 301
File: docs/CUSTOM_TOOLS.md:54-62
Timestamp: 2025-10-03T22:11:46.002Z
Learning: In Unity MCP, the `description` parameter in the `mcp_for_unity_tool` decorator is technically optional but should always be included as a best practice. Without it, there's a higher chance that MCP clients will not parse the tool correctly. All Unity MCP tools should include the description in the decorator for compatibility.
📚 Learning: 2025-10-03T22:11:46.002Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 301
File: docs/CUSTOM_TOOLS.md:54-62
Timestamp: 2025-10-03T22:11:46.002Z
Learning: In Unity MCP, the `description` parameter in the `mcp_for_unity_tool` decorator is technically optional but should always be included as a best practice. Without it, there's a higher chance that MCP clients will not parse the tool correctly. All Unity MCP tools should include the description in the decorator for compatibility.
Applied to files:
Server/src/services/tools/manage_gameobject.pyServer/src/services/tools/manage_material.pyServer/src/services/tools/manage_script.pyServer/src/services/tools/manage_scene.py
📚 Learning: 2025-11-05T18:23:12.349Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Applied to files:
Server/src/services/tools/manage_script.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (5)
Server/src/services/tools/manage_gameobject.py (1)
15-21: Annotations and description correctly applied.The
destructiveHint=Trueis appropriate since this tool supports destructive operations (create, modify, delete, etc.), even though some actions are read-only. The description clearly documents which actions fall into each category, enabling LLMs to make informed decisions.Server/src/services/tools/manage_material.py (1)
17-23: Annotations properly configured.The tool annotation correctly marks this as destructive since most operations modify material state. The description comprehensively categorizes all 7 actions (ping, get_material_info as read-only; the rest as destructive).
Server/src/services/tools/manage_scene.py (1)
12-18: Annotations correctly applied with accurate action classification.The description properly categorizes screenshot as read-only (captures without modifying) and scene-modifying operations (create, load, save) as destructive. Good alignment between the action literals and documented behavior.
Server/src/services/tools/manage_script.py (2)
67-86: Well-documented tool with appropriate destructive hint.The multi-line description provides excellent guidance for safe usage, including the recommended workflow and notes about alternative tools. The
destructiveHint=Truecorrectly reflects that this tool modifies script files.
377-383: All tool annotations correctly configured.Each tool has appropriate hints:
create_script,delete_script,manage_script→destructiveHint=Truevalidate_script,manage_script_capabilities,get_sha→readOnlyHint=TrueThe descriptions clearly document tool purposes. Based on learnings, including
descriptionin the decorator is a best practice for MCP client compatibility.Also applies to: 428-434, 456-462, 503-509, 577-590, 615-621
|
Read console can't do anything destructive can it? |
|
You're right! Clearing the console affects ephemeral UI state, not project data – it's not "destructive" in the MCP sense (irreversible data loss/modification). Changed to |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Server/src/services/tools/read_console.py (1)
15-21: Annotations and description look good!The
readOnlyHint=Trueannotation accurately reflects that this tool doesn't modify persistent project state, and the updated description provides helpful guidance about thecountparameter format. The defensive coercion code (lines 67-82) already handles both int and string inputs correctly.Optional: Consider clarifying "read-only" scope in the description
Since the tool supports both
action='get'(read-only) andaction='clear'(modifies ephemeral console UI), you might optionally expand the description to clarify that "read-only" refers to project/persistent state, not the console buffer itself. For example:- description="Gets messages from or clears the Unity Editor console. Note: For maximum client compatibility, pass count as a quoted string (e.g., '5').", + description="Gets messages from or clears the Unity Editor console (clears ephemeral UI state only, no project modifications). Note: For maximum client compatibility, pass count as a quoted string (e.g., '5').",This is purely for explicitness and not required given the maintainer's deliberate decision on the hint semantics.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Server/src/services/tools/read_console.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 301
File: docs/CUSTOM_TOOLS.md:54-62
Timestamp: 2025-10-03T22:11:46.002Z
Learning: In Unity MCP, the `description` parameter in the `mcp_for_unity_tool` decorator is technically optional but should always be included as a best practice. Without it, there's a higher chance that MCP clients will not parse the tool correctly. All Unity MCP tools should include the description in the decorator for compatibility.
📚 Learning: 2025-10-03T22:11:46.002Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 301
File: docs/CUSTOM_TOOLS.md:54-62
Timestamp: 2025-10-03T22:11:46.002Z
Learning: In Unity MCP, the `description` parameter in the `mcp_for_unity_tool` decorator is technically optional but should always be included as a best practice. Without it, there's a higher chance that MCP clients will not parse the tool correctly. All Unity MCP tools should include the description in the decorator for compatibility.
Applied to files:
Server/src/services/tools/read_console.py
🧬 Code graph analysis (1)
Server/src/services/tools/read_console.py (2)
Server/src/transport/unity_transport.py (1)
send_with_unity_instance(78-103)Server/src/transport/legacy/unity_connection.py (1)
async_send_command_with_retry(753-785)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: Sourcery review
🔇 Additional comments (1)
Server/src/services/tools/read_console.py (1)
7-8: LGTM!The
ToolAnnotationsimport is correctly added and necessary for the annotations block below.
|
@triepod-ai please update with the main branch, we've had some changes. Why did your I looked at the docs: https://gofastmcp.com/servers/tools#mcp-annotations While some of the tools can destroy resources, are they destructive? If I'm adding a GameObject, then manage_gameobject shouldn't be treated as destructive, though it can delete. I'd rather use that annotation if the tool only destroyed something. I'm happy to hear counterarguments, I didn't find the world of docs or use cases for this feature |
triepod-ai
force-pushed
the
feat/add-tool-annotations
branch
from
9752549 to
2ae5e7d
Compare
|
Thanks for the thorough feedback! You're absolutely right about the annotation semantics. Branch update: Done - rebased with main. uv.lock change: This changed because the MCP SDK was bumped from Annotation semantics: You're correct. I've updated the PR to follow the three-tier classification from the MCP spec:
Tools like Changed to title-only:
Kept
|
|
@triepod-ai -- Also, just curious -- is your account responding automatically via AI? |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
Server/src/services/tools/manage_scene.py (1)
34-35: Consider validatingscreenshot_super_sizeconstraint.The parameter description specifies "integer ≥1", but the implementation at line 50 only coerces to int without validating the lower bound. Invalid values (≤0) could be passed to Unity.
Consider adding validation after coercion:
🔎 Proposed validation for screenshot_super_size
coerced_super_size = coerce_int(screenshot_super_size, default=None) + if coerced_super_size is not None and coerced_super_size < 1: + return {"success": False, "message": "screenshot_super_size must be ≥1"}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Server/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Server/src/services/tools/batch_execute.pyServer/src/services/tools/debug_request_context.pyServer/src/services/tools/execute_custom_tool.pyServer/src/services/tools/execute_menu_item.pyServer/src/services/tools/find_in_file.pyServer/src/services/tools/manage_asset.pyServer/src/services/tools/manage_editor.pyServer/src/services/tools/manage_gameobject.pyServer/src/services/tools/manage_material.pyServer/src/services/tools/manage_prefabs.pyServer/src/services/tools/manage_scene.pyServer/src/services/tools/manage_script.pyServer/src/services/tools/manage_shader.pyServer/src/services/tools/read_console.pyServer/src/services/tools/run_tests.pyServer/src/services/tools/script_apply_edits.pyServer/src/services/tools/set_active_instance.py
🚧 Files skipped from review as they are similar to previous changes (8)
- Server/src/services/tools/manage_asset.py
- Server/src/services/tools/script_apply_edits.py
- Server/src/services/tools/debug_request_context.py
- Server/src/services/tools/find_in_file.py
- Server/src/services/tools/read_console.py
- Server/src/services/tools/batch_execute.py
- Server/src/services/tools/execute_menu_item.py
- Server/src/services/tools/run_tests.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 301
File: docs/CUSTOM_TOOLS.md:54-62
Timestamp: 2025-10-03T22:11:46.002Z
Learning: In Unity MCP, the `description` parameter in the `mcp_for_unity_tool` decorator is technically optional but should always be included as a best practice. Without it, there's a higher chance that MCP clients will not parse the tool correctly. All Unity MCP tools should include the description in the decorator for compatibility.
📚 Learning: 2025-10-03T22:11:46.002Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 301
File: docs/CUSTOM_TOOLS.md:54-62
Timestamp: 2025-10-03T22:11:46.002Z
Learning: In Unity MCP, the `description` parameter in the `mcp_for_unity_tool` decorator is technically optional but should always be included as a best practice. Without it, there's a higher chance that MCP clients will not parse the tool correctly. All Unity MCP tools should include the description in the decorator for compatibility.
Applied to files:
Server/src/services/tools/set_active_instance.pyServer/src/services/tools/manage_gameobject.pyServer/src/services/tools/execute_custom_tool.pyServer/src/services/tools/manage_script.pyServer/src/services/tools/manage_scene.pyServer/src/services/tools/manage_material.pyServer/src/services/tools/manage_prefabs.pyServer/src/services/tools/manage_editor.pyServer/src/services/tools/manage_shader.py
📚 Learning: 2025-11-05T18:23:12.349Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 368
File: MCPForUnity/UnityMcpServer~/src/resources/menu_items.py:15-15
Timestamp: 2025-11-05T18:23:12.349Z
Learning: In Unity MCP, the `name` parameter in the `mcp_for_unity_resource` decorator is the external API name exposed to MCP clients (LLMs, AI agents). The command string passed to `async_send_command_with_retry` or `async_send_with_unity_instance` (e.g., "get_menu_items") is the internal command identifier that must match the C# side. These are decoupled, allowing external API naming to evolve independently of internal command routing.
Applied to files:
Server/src/services/tools/execute_custom_tool.pyServer/src/services/tools/manage_script.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (21)
Server/src/services/tools/manage_prefabs.py (2)
4-4: LGTM!The ToolAnnotations import is correctly added and follows the pattern established across the PR.
15-17: Missing destructiveHint annotation inconsistent with PR objectives.The PR summary explicitly lists
manage_prefabsas one of 18 tools marked withdestructiveHint=true, but the annotations block here contains only the title. Since this tool can perform destructive operations (save_open_stage,create_from_gameobject), the absence ofdestructiveHint=Trueappears inconsistent with the stated PR goals of conservative annotation choices for tools that can modify state.Additionally, per the PR comments, dual-purpose routers were updated in commit 79c5ec7 to clarify which actions are destructive versus read-only in their descriptions. Consider enhancing the description to specify which of the four actions modify project state.
Suggested fix
@mcp_for_unity_tool( - description="Performs prefab operations (open_stage, close_stage, save_open_stage, create_from_gameobject).", + description="Performs prefab operations (open_stage, close_stage, save_open_stage, create_from_gameobject). Destructive actions: save_open_stage, create_from_gameobject.", annotations=ToolAnnotations( title="Manage Prefabs", + destructiveHint=True, ), )Server/src/services/tools/manage_gameobject.py (2)
6-6: LGTM: Import addition is correct.The ToolAnnotations import from mcp.types is properly added and necessary for the annotations parameter in the decorator.
16-19: Verify the omission of destructiveHint annotation.The decorator adds a title annotation and clarifies read-only vs. modifying actions in the description, but notably omits both
destructiveHintandreadOnlyHint. According to the PR objectives,manage_gameobjectwas initially included in the list of 18 tools marked as destructive.Given that this tool can perform clearly destructive operations (
delete,remove_component,set_component_property), and the PR rationale states that destructiveHint is kept "as a conservative default for tools that can perform destructive operations" to ensure clients surface appropriate warnings, the absence ofdestructiveHint=Trueappears inconsistent.Was the omission of
destructiveHintintentional based on reviewer feedback suggesting dual-purpose tools shouldn't be marked destructive, or was it an oversight?Suggested addition if destructive marking is appropriate:
🔎 Add destructiveHint annotation
@mcp_for_unity_tool( description="Performs CRUD operations on GameObjects and components. Read-only actions: find, get_components, get_component. Modifying actions: create, modify, delete, add_component, remove_component, set_component_property, duplicate, move_relative.", annotations=ToolAnnotations( title="Manage GameObject", + destructiveHint=True, ), )Server/src/services/tools/manage_scene.py (4)
4-4: LGTM!The import aligns with the PR objectives to add MCP tool annotations using mcp>=1.16.0.
13-18: Verify the intentional omission of destructive/readOnly hints for dual-purpose tools.The decorator includes a clear title and differentiates read-only from modifying actions in the description, but does not include
destructiveHintorreadOnlyHintin theToolAnnotations. According to the PR objectives, this tool was initially marked as destructive, but the approach was refined based on reviewer feedback to clarify behavior in descriptions for dual-purpose routers.Confirm this approach aligns with the final annotation strategy for tools that perform both read-only and modifying operations.
49-84: LGTM!The parameter coercion and conditional parameter building logic is clean and consistent. All optional parameters are properly checked before being added to the params dictionary, and the mapping to Unity-side naming conventions is appropriate.
86-95: LGTM!The response handling properly uses the centralized Unity interaction helper, unwraps successful responses into a user-friendly format, and provides structured error handling for both Unity-side and Python-side failures.
Server/src/services/tools/manage_shader.py (1)
5-5: LGTM! Import and description updates are appropriate.The ToolAnnotations import is correctly added, and the description update effectively clarifies which actions are read-only versus modifying, aligning with the PR's approach to document dual-purpose tool behavior.
Also applies to: 14-14
Server/src/services/tools/manage_material.py (1)
7-8: The import statement is correct and compatible with the declared dependency versionmcp>=1.16.0. TheToolAnnotationsclass is properly available inmcp.types, and this import pattern is consistently used across 15+ tool files throughout the codebase.Server/src/services/tools/set_active_instance.py (1)
5-6: LGTM!The import of
ToolAnnotationsfrommcp.typesis correct and necessary for the decorator annotations below.Server/src/services/tools/manage_editor.py (2)
15-15: Excellent description enhancement.The updated description clearly separates read-only actions from modifying actions, improving tool discoverability and helping MCP clients understand the tool's dual-purpose nature.
16-18: The absence ofdestructiveHintappears intentional and consistent with similar tools.While the tool description explicitly lists modifying actions (
add_tag,remove_tag,add_layer,remove_layer,play,pause,stop), the codebase shows a consistent pattern: dual-purpose tools likemanage_scene,manage_material, andmanage_prefabsalso omitdestructiveHintdespite having modifying operations. ThedestructiveHintannotation appears reserved for tools that are primarily destructive (e.g.,run_tests,script_apply_edits), not for mixed read-write tools. This design decision differs from a "conservative approach" of marking all state-modifying tools; if broader marking is intended, this would need to be applied consistently across all similar manage_* tools, not justmanage_editor.Server/src/services/tools/execute_custom_tool.py (1)
16-18: Consider adding destructiveHint for consistency with PR's conservative approach.The annotations currently include only a title. According to the PR objectives,
execute_custom_toolwas initially marked withdestructiveHint=Trueas part of the conservative default for tools that can modify state. Since this tool executes arbitrary custom tools registered by Unity—which could perform destructive operations—the absence ofdestructiveHintappears inconsistent with the stated approach.Was the destructive hint intentionally omitted after review discussions, or is this an oversight?
🔎 Suggested addition if destructiveHint is intended
annotations=ToolAnnotations( title="Execute Custom Tool", + destructiveHint=True, ),Server/src/services/tools/manage_script.py (7)
67-86: LGTM!The annotations are appropriate:
destructiveHint=Truecorrectly reflects that this tool modifies script content. The detailed description provides clear guidance on the recommended workflow.
377-390: LGTM!The annotations and signature changes are correct. The new parameters (
contents,script_type,namespace) are properly integrated into the function body, anddestructiveHint=Trueappropriately reflects the tool's script-creation behavior.
428-434: LGTM!The
destructiveHint=Trueannotation correctly reflects that this tool deletes scripts, which is an irreversible operation.
456-470: LGTM!The annotations and signature changes are correct. The
readOnlyHint=Trueappropriately reflects that validation doesn't modify state, and the new parameters (level,include_diagnostics) are properly integrated.
576-589: LGTM!The
readOnlyHint=Trueannotation correctly reflects that this tool only retrieves capability metadata without modifying state. The detailed description clearly documents the returned data structure.
614-620: LGTM!The
readOnlyHint=Trueannotation appropriately reflects that this tool only retrieves SHA metadata without returning or modifying file contents.
7-7: TheToolAnnotationsimport frommcp.typesis correct and properly supported. The project specifiesmcp>=1.16.0in dependencies, andToolAnnotationsis available in that version and later for adding metadata hints (destructiveHint, readOnlyHint, idempotentHint, openWorldHint, title) to tool definitions.
Dead internet theory is alive and well lol |
|
Good catch on To answer your question: I'm using Claude Code interactively as an AI-assisted development tool - it's not running automatically. I use it to help analyze codebases, draft changes, and iterate on review feedback. All contributions are manually reviewed and submitted by me. |
|
@triepod-ai I probably wasn't clear, I don't know if MCP docs suggests your original approach was correct i.e. we should add the destructive hint if the tool MAY perform a destructive action: https://modelcontextprotocol.io/legacy/concepts/tools?utm_source=chatgpt.com#available-tool-annotations. Apologies for the mix up. Can you re-apply to tool that may destroy a resource in Unity? Otherwise, this PR is fine with me @dsarno |
|
Thanks for the clarification @msanatan! I've re-applied Tools with
Kept as-is (no destructiveHint):
Commit: 1ad17a5 |
|
Good by me. Thanks @triepod-ai. Let's merge after #499 and add manage_scriptable _objects to list of annotated (destructive). |
triepod-ai
force-pushed
the
feat/add-tool-annotations
branch
from
1ad17a5 to
6c6cd13
Compare
|
Thanks @dsarno! I've rebased with main and added annotations to @mcp_for_unity_tool(
description="Creates and modifies ScriptableObject assets using Unity SerializedObject property paths.",
annotations=ToolAnnotations(
title="Manage Scriptable Object",
destructiveHint=True,
),
)Ready to merge after #499! 🚀 |
|
@triepod-ai Sorry -- a lot of activity over here. If you can fix the conflicts one more time I'll merge as soon as you're done. Thanks. |
Add readOnlyHint and destructiveHint annotations to all 23 tools
to help LLMs better understand tool behavior and make safer decisions.
Changes:
- Added readOnlyHint: true to 5 read-only tools:
- debug_request_context, find_in_file, validate_script,
manage_script_capabilities, get_sha
- Added destructiveHint: true to 18 tools that modify data:
- All CRUD tools, execution tools, and state-modifying tools
- Added title annotations for human-readable display
- Imported ToolAnnotations from mcp.types in all tool files
This improves tool safety metadata for MCP clients.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Address Sourcery AI review feedback by updating descriptions for batch_execute and manage_* tools to clarify their safety characteristics depend on the specific actions/operations being performed. Updated tools: - batch_execute: clarifies safety depends on contained tools - read_console: clarifies 'get' is read-only, 'clear' is destructive - manage_asset: lists read-only vs destructive actions - manage_gameobject: lists read-only vs destructive actions - manage_scene: lists read-only vs destructive actions - manage_editor: lists read-only vs destructive actions - manage_material: lists read-only vs destructive actions - manage_shader: lists read-only vs destructive actions - manage_script: lists read-only vs destructive actions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Address maintainer feedback on annotation semantics. Tools that have both read-only and modifying actions based on the 'action' parameter should not be marked as destructive. Changed to title-only annotations: - manage_gameobject (find/get vs create/modify/delete) - manage_scene (get_hierarchy vs create/load/save) - manage_prefabs (open_stage vs save/create) - manage_material (ping/get vs create/set) - manage_shader (read vs create/update/delete) - manage_editor (telemetry_status vs play/pause) - manage_script router (read vs create/delete) - batch_execute (depends on contained operations) - execute_menu_item (behavior varies by menu item) - execute_custom_tool (behavior varies by custom tool) - read_console (get vs clear - ephemeral UI state) Kept destructiveHint=True for always-destructive tools: - apply_text_edits (always writes files) - create_script (always creates files) - delete_script (always deletes files) - script_apply_edits (always writes files) - run_tests (always executes tests) - set_active_instance (always modifies session)
set_active_instance is a session-scoped operation that switches which Unity instance is active. It doesn't persist data and is easily reversible, so it should not be marked as destructive.
Per maintainer feedback referencing MCP docs, tools should have destructiveHint=True if they MAY perform a destructive action, even if they also support read-only operations. Re-applied to tools that can destroy Unity resources: - manage_gameobject, manage_scene, manage_prefabs - manage_material, manage_shader, manage_script - manage_asset, batch_execute - execute_menu_item, execute_custom_tool Kept as-is (no destructiveHint): - read_console (clearing is ephemeral UI state) - set_active_instance (session toggle, not destructive) - manage_editor (no destructive actions) 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Per reviewer request (@dsarno), add ToolAnnotations with destructiveHint=True to manage_scriptable_object tool since it can create/modify ScriptableObject assets. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Add readOnlyHint and destructiveHint annotations to tools added since the last commit: - refresh_unity: destructiveHint=True (triggers asset DB refresh) - run_tests_async: destructiveHint=True (starts test execution) - get_test_job: readOnlyHint=True (queries job status only) Also regenerate uv.lock after rebase.
triepod-ai
force-pushed
the
feat/add-tool-annotations
branch
from
6c6cd13 to
43eb921
Compare
|
@dsarno Done! Conflicts resolved and rebased onto main. Changes in this update:
Ready for merge whenever you are! 🚀 |
|
FYI this seems to have broken tests on the main branch: https://github.com/CoplayDev/unity-mcp/actions/runs/20733432481/job/59525976866 |
|
I fixed this in the gameobject PR fyi. Had to do with the upgrade to mcp SDK, which is overall a good thing anyway. |
I just rebased my fork and it still fails for me: https://github.com/msanatan/unity-mcp/actions/runs/20736274387. I'll check it out |
|
Yes I haven't merged the big gameobject PR that fixes it yet. You're welcome to patch tho. |
Nope, it's just another reminder that it's my bed time lol |
|
@triepod-ai if you're on LinkedIn, Twitter/X - share your profile and we'll tag you for your contributions! Completely optional of course |
|
@msanatan thanks!! appreciate it!! glad to be able to contribute! https://www.linkedin.com/in/bryan-thompson-it/ |
4 participants
Summary
Adds MCP tool annotations (
readOnlyHint,destructiveHint,title) to all 23 tools to help LLMs better understand tool behavior and make safer decisions about tool execution.Changes
Added
readOnlyHint: trueto 5 read-only tools:debug_request_context- Returns current FastMCP request contextfind_in_file- Searches files with regex patternsvalidate_script- Validates C# scripts and returns diagnosticsmanage_script_capabilities- Gets supported operations and limitsget_sha- Gets SHA256 and metadata for scriptsAdded
destructiveHint: trueto 18 tools that modify state:manage_asset,manage_gameobject,manage_material,manage_prefabs,manage_scene,manage_shader,manage_editorapply_text_edits,create_script,delete_script,manage_script,script_apply_editsexecute_menu_item,execute_custom_tool,batch_execute,run_testsset_active_instance,read_console(can clear console)Added
titleannotations for human-readable displayImported
ToolAnnotationsfrommcp.typesin all tool filesWhy This Matters
Tool annotations are advisory hints for MCP clients that enable:
readOnlyHint: truewithout user confirmationTesting
Tool Annotation Summary
🤖 Generated with Claude Code
Summary by Sourcery
Add MCP ToolAnnotations metadata to Unity MCP tools to indicate read-only vs destructive behavior and provide human-readable titles for safer client-side usage.
New Features:
Enhancements:
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.