Skip to content

Conversation

@dsarno
Copy link
Collaborator

@dsarno dsarno commented Dec 11, 2025

  1. Fix script path handling and FastMCP Context API usage (1628474)
  • Script Path Doubling Bug: Fixed ManageScript.TryResolveUnderAssets to properly
    handle Assets/ prefix. Previously, paths like Assets/Script.cs would incorrectly
    create files at Assets/Assets/Script.cs due to prefix duplication. Now correctly
    strips the prefix and creates files at the intended location.
  • FastMCP Context API: Changed ctx.warn() to ctx.warning() in manage_asset to match
    the FastMCP Context API, preventing AttributeError crashes.
  1. Fix manage_asset error handling to use ctx.error (1d9ed01)
  • Error Severity: Further refined error handling by changing ctx.warning() to
    ctx.error() for property parse errors in manage_asset. This ensures parse failures
    (e.g., invalid ScriptableObject properties) are properly reported as errors rather
    than warnings, improving error visibility and semantics.

Combined Impact: These fixes resolve crashes when creating ScriptableObjects with
invalid properties and ensure script files are created at the correct paths when
using the Assets/ prefix.

Summary by CodeRabbit

  • Bug Fixes
    • Asset path resolution now handles various path formats and edge cases with improved robustness to prevent unexpected failures
    • Properties parsing error handling enhanced with proper error-level notifications instead of warnings, providing clearer feedback and better diagnostics for users

✏️ Tip: You can customize this high-level summary in your review settings.

dsarno and others added 2 commits December 10, 2025 16:15
1. Fix script path doubling when Assets prefix is used
   - ManageScript.TryResolveUnderAssets now properly handles both Assets and Assets/ prefixes
   - Previously, paths like Assets/Script.cs would create files at Assets/Assets/Script.cs
   - Now correctly strips the prefix and creates files at the intended location

2. Fix FastMCP Context API call in manage_asset
   - Changed ctx.warn() to ctx.warning() to match FastMCP Context API
   - Fixes AttributeError when manage_asset encounters property parse errors
   - Affects ScriptableObject creation and other asset operations with invalid properties
Changed ctx.warning to ctx.error for property parse errors in manage_asset
tool to properly handle error cases. This ensures parse errors are reported
as errors rather than warnings, and fixes compatibility with FastMCP Context API.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 11, 2025

Walkthrough

The PR makes two targeted improvements: enhanced path normalization in ManageScript's TryResolveUnderAssets to handle both "Assets" and "Assets/..." prefixes more robustly, and elevated error reporting in manage_asset.py from warnings to errors when properties parsing fails.

Changes

Cohort / File(s) Summary
Path Normalization
MCPForUnity/Editor/Tools/ManageScript.cs
Enhanced TryResolveUnderAssets to handle "Assets" (case-insensitive) as root and "Assets/..." prefix removal for improved robustness.
Error Handling
Server/src/services/tools/manage_asset.py
Changed error reporting from ctx.warn() to ctx.error() when properties parsing fails; behavior and return value unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Path normalization logic in TryResolveUnderAssets is straightforward prefix/equality matching with minimal conditional branches
  • Error handling change is a direct method substitution with no control flow alteration

Possibly related PRs

Poem

🐰 A rabbit hops through Assets with glee,
Paths now handled more carefully,
And when errors appear in the code,
We cry out loud down the logging road! 🌾

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title references the main changes: script path Assets prefix handling and ctx warn/error API usage fixes. Both are clearly documented as key objectives in the PR.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 237b26e and 1d9ed01.

📒 Files selected for processing (2)
  • MCPForUnity/Editor/Tools/ManageScript.cs (1 hunks)
  • Server/src/services/tools/manage_asset.py (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.

Applied to files:

  • MCPForUnity/Editor/Tools/ManageScript.cs
🧬 Code graph analysis (1)
Server/src/services/tools/manage_asset.py (1)
Server/tests/integration/test_helpers.py (1)
  • error (46-47)
🔇 Additional comments (2)
MCPForUnity/Editor/Tools/ManageScript.cs (1)

68-78: LGTM! The path normalization fix correctly prevents Assets/ prefix duplication.

The new logic properly handles both exact "Assets" and "Assets/..." prefixes:

  • "Assets" → treated as root (empty string)
  • "Assets/Script.cs" → normalized to "Script.cs"
  • "Scripts/Foo.cs" → unchanged

This correctly fixes the reported duplication bug where "Assets/Script.cs" was previously becoming "Assets/Assets/Script.cs".

Server/src/services/tools/manage_asset.py (1)

82-84: The ctx.error() method is correct and properly used.

The severity escalation from warning to error is appropriate for this scenario. Parse errors prevent operation success (the function immediately returns {"success": False}), matching error-level logging severity. The fastmcp Context API (version 2.13.0+) supports the error() method as documented, and it is already used elsewhere in the codebase for similar error conditions.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@dsarno dsarno merged commit 2b23af4 into CoplayDev:main Dec 11, 2025
1 check passed
choej2303 pushed a commit to choej2303/unity-mcp-ide that referenced this pull request Dec 13, 2025
* Fix script path handling and FastMCP Context API usage

1. Fix script path doubling when Assets prefix is used
   - ManageScript.TryResolveUnderAssets now properly handles both Assets and Assets/ prefixes
   - Previously, paths like Assets/Script.cs would create files at Assets/Assets/Script.cs
   - Now correctly strips the prefix and creates files at the intended location

2. Fix FastMCP Context API call in manage_asset
   - Changed ctx.warn() to ctx.warning() to match FastMCP Context API
   - Fixes AttributeError when manage_asset encounters property parse errors
   - Affects ScriptableObject creation and other asset operations with invalid properties

* Fix manage_asset error handling to use ctx.error

Changed ctx.warning to ctx.error for property parse errors in manage_asset
tool to properly handle error cases. This ensures parse errors are reported
as errors rather than warnings, and fixes compatibility with FastMCP Context API.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.5 <[email protected]>

---------

Co-authored-by: Claude Sonnet 4.5 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant