feat: Help older custom tools migrate to V3 automatically#1125
Merged
Conversation
Detect legacy third-party custom tool API usage during setup so suppressed update popups still surface when migration work is required. Add migration rules and file-service coverage to rewrite legacy tool contracts and asmdef references to the V3 ToolContracts assembly.
Keep supported tool attribute arguments and skip inert comments or strings so the V3 migration does not change tool visibility or rewrite non-code examples.
Limit source rewrites to legacy tool migration markers, handle McpTool entries inside shared attribute lists, and avoid scanning for migration targets when the recorded setup version is unchanged.
Rewrite qualified legacy tool attributes and keep the Application assembly reference when CustomToolManager usages migrate to the V3 registrar.
Scan C# attribute lists with source awareness and include the remaining renamed helper types so realistic V2 custom tools migrate to compilable V3 code.
Keep CustomToolManager metadata callers compilable by adding the Domain asmdef reference and rewriting explicit ToolInfo declarations.
Apply contract renames across legacy tool assemblies so schema and helper files without local using directives still compile after migration.
Global legacy usings can make a whole third-party asmdef eligible for contract migration even when manual registration lives in a separate file. Track registrar usage at the assembly level so migrated registrar calls get the Application and Domain references they need.
Custom tools can rely on global legacy usings without an asmdef, so treat no-asmdef sources as one implicit assembly during migration. Use migration-local unique temp and backup paths so project sidecar files are not overwritten or deleted while source files are rewritten.
Legacy custom tools can shorten the old namespace with a C# alias before calling CustomToolManager. Rewrite those alias-qualified registrar calls explicitly so the migration does not create invalid qualified V3 registrar names.
No-asmdef Unity projects still compile runtime and editor sources into different predefined assemblies. Keep those implicit assemblies separate during global-using migration, and finish alias-qualified legacy rewrites for tool attributes and registrar metadata.
Legacy tools can use global-qualified attributes or split ToolInfo metadata away from registration calls. Handle those forms so source rewrites clean the old attribute arguments and asmdefs receive Domain references when metadata types are migrated.
Unity asmref folders compile into their referenced asmdef, so include asmref resolution when deciding which assembly owns legacy source. Also limit assembly-wide global-using rewrites to files that actually use legacy tool API shapes, avoiding unrelated project type renames.
Interpolated strings can contain real C# expressions that reference legacy tool APIs. Keep interpolation holes visible to the migration mask so registrar and metadata references inside those expressions are rewritten while literal string text stays untouched.
Migration detection needs to follow Unity assembly routing and C# code masking closely enough to avoid leaving V2 API references behind. - Treat generic legacy type references as assembly-scoped API usage - Keep interpolation holes in verbatim interpolated strings visible to rewrites - Resolve C# files to the most specific asmdef or asmref source directory
Package Manager content is not project-owned source, so the migration should avoid rewriting package folders in place. Limit the automatic scan to project assets and keep package contents untouched.
Raw interpolated string holes can contain legacy API calls that need the same syntax-aware rewriting as regular and verbatim interpolated strings. Keep raw literal text inert while allowing code inside interpolation holes to migrate.
Migration should not rename unrelated project types just because their names match old tool contract types. Limit contract renames to unqualified legacy use, the old namespace, or aliases that explicitly point to the old namespace.
Bare McpTool attributes can belong to unrelated packages, while split files in a legacy assembly can still rely on global uLoopMCP usings. Require legacy context before rewriting bare attributes and detect all renamed contract type references when planning split-file migration.
Assemblies detected through a separate global uLoopMCP using can have sibling files that only contain shorthand McpTool attributes. Treat those attributes as assembly-scoped migration targets so the migrated global using does not leave unresolved legacy attributes behind.
Unqualified legacy-like names can belong to unrelated project code, and namespace prefixes can resemble the old namespace without being it. Require an actual legacy namespace or qualified attribute marker for direct source migration and anchor namespace replacement to full namespace tokens.
Drop the removed V2 ToolInfo description argument during custom tool migration and keep nested string literals inside interpolation holes inert so migration rewrites only executable code.
Keep the V2 displayDevelopmentOnly argument when dropping the removed ToolInfo description parameter so migrated registrar metadata preserves tool visibility and compiles against the V3 constructor.
Limit assembly-scoped legacy rewrites to global using declarations so file-scoped imports do not migrate sibling files, and avoid treating current V3 ToolInfo constructor arguments as the removed V2 description signature.
Separate predefined firstpass assemblies during global-using migration and exclude only the project-root Packages directory so Assets folders with the same name can still be migrated.
Scan only Unity-compiled Assets sources and remove legacy ToolInfo description arguments even when descriptions are passed through variables or named arguments.
Carry global legacy namespace aliases across assembly-scoped migration so split files can be rewritten, and keep current ToolInfo constructor calls with arbitrary local variable names intact.
Rewrite constructor arguments only when the source constructor is a legacy ToolInfo reference, including type aliases, so current V3 ToolInfo calls keep their schema argument intact.
Detect already-migrated UnityCliLoopToolRegistrar usage when rewriting legacy editor references so mixed assemblies keep the Application and Domain dependencies they still need.
Avoid rewriting using-alias left-hand identifiers while migrating their targets so same-name legacy aliases remain valid after ToolInfo and helper type rewrites.
Require legacy API context before treating CustomToolManager as the V2 registrar, so unrelated project-owned type names do not trigger asmdef dependency churn.
Reduce the intensity of the migration alert background and border so the warning stays visible without dominating the setup wizard.
Make the migration alert outline slightly stronger so the softened warning color still has clear visual separation.
Scan third-party tool migration targets in small batches with progress so opening the setup wizard does not block the editor on large projects. Cache preview results between refreshes, keep migration detection scoped to likely source files, and surface a progress bar while the check runs.
Treat the shared Application GUID as a valid V3 reference unless legacy C# source confirms that the asmdef still points at the old editor assembly. Only add Domain references when migrated or current source actually uses Domain APIs, preventing setup migration prompts for already-compiling project assemblies.
Run a lightweight V3 custom tool migration presence check only when the setup wizard sees a package version change, then start the full preview only for windows opened with detected migration targets. This keeps manual setup wizard opens responsive while still surfacing migration work during upgrades.
Separate V3 custom tool migration from Setup Wizard so setup auto-show state and migration detection no longer share one window lifecycle. The migration wizard now owns its menu item, startup target check, preview progress, refresh, and migrate action while Setup Wizard returns to CLI and skill setup only.
Tell users that closing the dedicated migration wizard is safe because it can be reopened later from the Unity CLI Loop menu.
Align the migration alert with the existing caution palette so the wizard matches the darker amber warning styling.
Use a lighter border weight so the migration wizard warning matches the requested visual balance.
Measure the migration wizard content after layout so the window shrinks or expands to the warning copy and footer hint instead of relying on a fixed height.
Use the Setup Wizard width as the migration wizard baseline and fit only the height to content so long migration messages wrap vertically instead of widening the window.
Show a dedicated disabled button label when no migration targets remain so the wizard communicates that there is nothing left to migrate.
Ensure the migration wizard refresh path rescans project files instead of returning a cached preview, so manually fixed or newly added tools are reflected while the Editor stays open.
Extend startup migration detection to catch already-rewritten V3 tool sources whose asmdefs still need current references, so the migration wizard appears for repairable partial states.
Limit startup migration detection to asmdefs that belong to assemblies with current API references, so unrelated malformed asmdefs do not block repairable migration targets.
Mirror assembly-scoped Domain global-using detection in the startup scan and avoid parsing unrelated asmdefs when previewing or applying repairable asmdef migrations.
GetRegisteredCustomTools returns ToolInfo from the Domain assembly, so migrated assemblies must receive the Domain asmdef reference even when source code only uses the registrar return value indirectly.
Remove obsolete description arguments from migrated ToolSettingsCatalogItem calls and carry global ToolInfo type aliases across split assembly files so migrated projects keep compiling after V3 API rewrites.
Global ToolInfo type aliases should not grant full legacy namespace context to sibling files. Limit them to alias constructor migration so unrelated bare project types stay untouched.
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR introduces a complete third-party tool V3 migration feature: a scanning service that detects and rewrites legacy tool APIs into current contracts, a wizard UI for guided migration, comprehensive test coverage of transformation rules and project scanning, and integration into the editor startup flow. ChangesThird-party Tool V3 Migration
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
🚥 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 |
![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
🧹 Nitpick comments (1)
Packages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWindow.cs (1)
190-193: ⚡ Quick winAdd Debug.Assert after registry lookup.
For defensive programming and fail-fast behavior, add a Debug.Assert after retrieving the use case from the registry to validate the composition root wired the dependency correctly.
🛡️ Proposed assertion to validate registry state
private void InitializeApplicationServices() { _thirdPartyToolMigrationUseCase = ThirdPartyToolMigrationUseCaseRegistry.GetRegisteredUseCase(); + Debug.Assert(_thirdPartyToolMigrationUseCase != null, "migration use case must be registered"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWindow.cs` around lines 190 - 193, After calling ThirdPartyToolMigrationUseCaseRegistry.GetRegisteredUseCase() in InitializeApplicationServices, add a Debug.Assert to validate that _thirdPartyToolMigrationUseCase is not null (use System.Diagnostics.Debug.Assert) so the composition root wired the dependency; include a short message like "ThirdPartyToolMigrationUseCase must be registered" to aid diagnosis and place the assert immediately after the assignment to _thirdPartyToolMigrationUseCase.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Packages/src/Editor/Domain/ThirdPartyToolMigrationData.cs`:
- Around line 13-22: The constructors for ThirdPartyToolMigrationPreview (and
the corresponding other struct around lines 56-65) currently assign the
caller-provided string[] directly to the FilePaths field, allowing external
mutation; change the constructors to defensively copy the array before storing
(e.g., if filePaths is non-null create a new string[] copy or use
Array.Copy/Clone, otherwise use Array.Empty<string>), and keep FilePaths as the
same public property type so callers see an immutable snapshot of the input.
In
`@Packages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWindow.cs`:
- Around line 330-348: The Debug.Assert in HandleMigrateThirdPartyTools assumes
ApplyMigration always returns Changed=true which is false when there's nothing
to migrate; remove or replace the assertion and explicitly handle the no-change
case by checking the ThirdPartyToolMigrationResult (result) and only calling
AssetDatabase.Refresh() when result.Changed is true, otherwise update the
UI/state to reflect "no changes" (e.g., still set _isMigrating = false and call
RefreshUI()); reference the HandleMigrateThirdPartyTools method, the
_thirdPartyToolMigrationUseCase.ApplyMigration call, the
ThirdPartyToolMigrationResult/result.Changed check, Debug.Assert,
AssetDatabase.Refresh(), _isMigrating and RefreshUI to locate where to implement
this conditional handling.
---
Nitpick comments:
In
`@Packages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWindow.cs`:
- Around line 190-193: After calling
ThirdPartyToolMigrationUseCaseRegistry.GetRegisteredUseCase() in
InitializeApplicationServices, add a Debug.Assert to validate that
_thirdPartyToolMigrationUseCase is not null (use
System.Diagnostics.Debug.Assert) so the composition root wired the dependency;
include a short message like "ThirdPartyToolMigrationUseCase must be registered"
to aid diagnosis and place the assert immediately after the assignment to
_thirdPartyToolMigrationUseCase.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5235234b-2111-4cb3-ac74-a14505488daa
⛔ Files ignored due to path filters (10)
Assets/Tests/Editor/ThirdPartyToolMigrationFileServiceTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/ThirdPartyToolMigrationRulesTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/ThirdPartyToolMigrationWizardWindowTests.cs.metais excluded by none and included by nonePackages/src/Editor/Application/UseCases/ThirdPartyToolMigrationUseCase.cs.metais excluded by none and included by nonePackages/src/Editor/Application/UseCases/ThirdPartyToolMigrationUseCaseRegistry.cs.metais excluded by none and included by nonePackages/src/Editor/Domain/ThirdPartyToolMigrationData.cs.metais excluded by none and included by nonePackages/src/Editor/Infrastructure/ThirdPartyToolMigration.metais excluded by none and included by nonePackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationFileService.cs.metais excluded by none and included by nonePackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationRules.cs.metais excluded by none and included by nonePackages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWindow.cs.metais excluded by none and included by none
📒 Files selected for processing (15)
Assets/Tests/Editor/OnionAssemblyDependencyTests.csAssets/Tests/Editor/SetupWizardWindowTests.csAssets/Tests/Editor/ThirdPartyToolMigrationFileServiceTests.csAssets/Tests/Editor/ThirdPartyToolMigrationRulesTests.csAssets/Tests/Editor/ThirdPartyToolMigrationWizardWindowTests.csPackages/src/Editor/Application/UseCases/ThirdPartyToolMigrationUseCase.csPackages/src/Editor/Application/UseCases/ThirdPartyToolMigrationUseCaseRegistry.csPackages/src/Editor/CompositionRoot/UnityCliLoopApplicationRegistration.csPackages/src/Editor/Domain/ThirdPartyToolMigrationData.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationFileService.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationRules.csPackages/src/Editor/Presentation/PresentationEditorStartup.csPackages/src/Editor/Presentation/Setup/SetupWizardWindow.csPackages/src/Editor/Presentation/Setup/SetupWizardWindow.ussPackages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWindow.cs
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 25 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWindow.cs">
<violation number="1" location="Packages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWindow.cs:302">
P2: Initial migration scan is always scheduled, ignoring `_shouldRefreshAfterCreateGui` and triggering unnecessary rescans.</violation>
</file>
<file name="Packages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationFileService.cs">
<violation number="1" location="Packages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationFileService.cs:1279">
P2: Guard `.asmdef`/`.asmref` JSON parsing so malformed project files don't crash the migration scan.
(Based on your team's feedback about validating user-editable external inputs.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Preserve migration preview/result snapshots against array mutation, keep no-op migration runs from refreshing assets, honor the initial refresh flag, and skip malformed assembly JSON during migration scans.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationFileService.cs">
<violation number="1" location="Packages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationFileService.cs:1451">
P2: `TryReadJsonObject` only handles parse errors, so file read failures can still throw and terminate migration scanning. Catch I/O read exceptions as well so callers can consistently skip unreadable files.
(Based on your team's feedback about handling read/parse failures together in JSON file polling/migration flows.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Treat assembly definition and reference read failures like malformed JSON so migration scans can continue when a user-editable file is locked or blocked.
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.
Summary
User Impact
Changes
Verification