fix: Make V3 migration safer and easier to run#1386
Merged
Conversation
Restrict V3 migration OUTPUT_FIELD candidates to jq selector text and PowerShell variables parsed from ConvertFrom-Json so Markdown C# snippets are not reported as uloop JSON parsing.
Remove the fragile V3 migration detector scripts and make the skill rely on repository search plus context inspection. Expand the reference so agents have the full migration map, false-positive guidance, and stale installed script cleanup coverage.
Update the migration wizard prompt copy so users can ask an AI agent to run the V3 CLI invocation migration skill, and add a clipboard button for the prompt.
Place the AI migration prompt after the skill install button so users see the copyable prompt in the requested flow, and cover the ordering with an EditMode test.
Keep the AI prompt text inside the collapsible section while leaving the copy action visible below it, and cover the UI hierarchy with an EditMode test.
Keep the Check button visible but disabled when no C# migration targets remain, so the completed state no longer offers a redundant Close action.
Keep current Application, Domain, and FirstPartyTools namespace references intact during C# migration so internal package code and tests are not rebound to ToolContracts by broad namespace replacement.
Avoid rewriting already-current ToolContracts references to fully qualified names when a ToolContracts using is available, while keeping legacy and qualified migrations resolvable.
Rewrite only moved public contract types to ToolContracts while preserving current Application, Domain, and FirstPartyTools references that still resolve valid APIs. This prevents the setup migration from rebinding already-current code and keeps completed projects as no-op migration candidates.
Carry current Domain namespace aliases through migration planning and rewrite alias-qualified moved contract types to ToolContracts. Also make the fast migration-target scanner report Domain global using asmdef repairs so the wizard does not miss reference-only fixes.
Ensure migration adds ToolContracts references when current Domain aliases and FirstPartyTools screenshot contract types are rewritten to ToolContracts, keeping sync, async, and fast target scans aligned.
When a file-local Application using participates in a migration to ToolContracts, keep the Application asmdef reference so the preserved using remains compilable without broad using-only repairs.
Only actual Domain using directives should permit bare contract type rewrites. Aliases and fully qualified Domain references now rewrite qualified names without corrupting unrelated bare identifiers.
Record Application asmdef requirements for current local Application usings so preserved Application-layer types remain compilable after legacy references are migrated away.
Record Domain asmdef requirements for current local Domain usings so Domain-layer types that stay in Domain remain compilable after migration checks.
Current FirstParty screenshot APIs can expose ToolContracts types in their public signatures, so migration now records the direct ToolContracts asmdef requirement while preserving the FirstParty call sites.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR extends the V3 third-party tool migration engine to track current-domain and current-application namespace aliases per assembly directory, rewires ChangesV3 Migration Engine, Wizard UI, Skill Tooling, and Tests
Sequence Diagram(s)sequenceDiagram
participant PlanBuilder as ThirdPartyToolMigrationPlanBuilder
participant Rules as ThirdPartyToolMigrationRules
participant CSharpRules as ThirdPartyToolMigrationCSharpRules
participant AsmdefRules as ThirdPartyToolMigrationAsmdefRules
participant TypeReplacement as TypeReplacementRules
PlanBuilder->>PlanBuilder: Lookup currentDomainAssemblyAliases from AssemblyScopedCurrentDomainAliasesByDirectory
PlanBuilder->>PlanBuilder: Compute hasAssemblyScopedCurrentDomainUsing from AssemblyScopedCurrentDomainDirectories
PlanBuilder->>Rules: MigrateCSharpSourceForLegacyAssembly(…, hasAssemblyScopedCurrentDomainUsing, currentDomainAssemblyAliases)
Rules->>CSharpRules: MigrateCSharpSourceForLegacyAssembly(…, domain params)
CSharpRules->>CSharpRules: Compute canPreserveBareCurrentToolContractsReferences, canMigrateBareCurrentDomainContractType
CSharpRules->>TypeReplacement: ReplaceLegacyApplicationTypeNamesInCode(…, currentApplicationNamespaceAliases, canPreserveBare…)
CSharpRules->>TypeReplacement: ReplaceCurrentDomainContractTypeNamesInCode(…, currentDomainNamespaceAliases, canMigrateBare…)
CSharpRules->>TypeReplacement: ReplaceLegacyFirstPartyScreenshotTypeNamesInCode(…, currentFirstPartyToolsNamespaceAliases, canPreserveBare…)
PlanBuilder->>AsmdefRules: AddRequiredCurrentAsmdefReferences(toolContractsRequired, applicationRequired, domainRequired, firstPartyRequired)
AsmdefRules->>AsmdefRules: Branch 1: if toolContractsRequired → AddRequiredCurrentAsmdefReference ToolContracts GUID
AsmdefRules->>AsmdefRules: Branch 2: if applicationRequired → AddRequiredCurrentAsmdefReference Application GUID
AsmdefRules->>AsmdefRules: Branch 3: if domainRequired → AddRequiredCurrentAsmdefReference Domain GUID
AsmdefRules->>AsmdefRules: Branch 4: if firstPartyRequired → AddRequiredCurrentAsmdefReference FirstParty GUID
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 docstrings
🧪 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Packages/src/TemporarySkills~/v3-cli-invocation-migration/Skill/references/first-party-v2-to-v3.md (1)
3-14: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueExcellent structure for a canonical migration reference. The Search Checklist, boolean rules, removed commands, and context rules provide clear guidance for agents and humans alike. The false-positive list (lines 109–115) is particularly strong in preventing over-migration.
Minor observation: Lines 11 and 13 each start three consecutive bullets with "Search" (flagged by static analysis). The repetition is intentional for a checklist but could be softened if readability is a priority (e.g., "Search for…", "Also look for…", "Check for…").
Also applies to: 25-26, 50-59, 101-115
🤖 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/TemporarySkills`~/v3-cli-invocation-migration/Skill/references/first-party-v2-to-v3.md around lines 3 - 14, The Search Checklist section contains consecutive bullet points that repetitively start with the word "Search" (particularly visible at lines 11-13, and also at lines 25-26, 50-59, and 101-115), which impacts readability. To improve readability while maintaining the checklist's clarity and intentional structure, vary the starting verbs and phrases of consecutive bullets. Replace repeated instances of "Search" with synonymous alternatives such as "Also look for", "Check for", "Review", "Verify", or similar action verbs that convey the same meaning but improve the natural flow of the checklist without losing the prescriptive guidance.
🤖 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/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationCSharpRules.cs`:
- Around line 273-275: The issue is that `canUseBareCurrentFirstPartyTools` is
being passed as the bare-preservation flag in the method call, but it's true for
any `CurrentFirstPartyToolsNamespaceRegex` match (including fully-qualified
references), not just when FirstPartyTools using statements are actually in
scope. This causes incorrect bare rewrites of legacy capture calls to
`EditorWindowCaptureUtility` even without appropriate FirstPartyTools usings,
producing uncompilable code. Instead of passing
`canUseBareCurrentFirstPartyTools` as the third parameter, create a proper
FirstPartyTools using detector (parallel to the existing Domain/Application
using detectors) that specifically checks for using or global using statements,
and pass that detector result as the bare-preservation flag to ensure bare
legacy calls are only rewritten when appropriate FirstPartyTools usings are
actually in scope.
---
Nitpick comments:
In
`@Packages/src/TemporarySkills`~/v3-cli-invocation-migration/Skill/references/first-party-v2-to-v3.md:
- Around line 3-14: The Search Checklist section contains consecutive bullet
points that repetitively start with the word "Search" (particularly visible at
lines 11-13, and also at lines 25-26, 50-59, and 101-115), which impacts
readability. To improve readability while maintaining the checklist's clarity
and intentional structure, vary the starting verbs and phrases of consecutive
bullets. Replace repeated instances of "Search" with synonymous alternatives
such as "Also look for", "Check for", "Review", "Verify", or similar action
verbs that convey the same meaning but improve the natural flow of the checklist
without losing the prescriptive guidance.
🪄 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: 995a5051-3165-4e8d-829b-b2ae9137fdee
⛔ Files ignored due to path filters (1)
Packages/src/TemporarySkills~/v3-cli-invocation-migration/Skill/scripts/detect-v3-cli-invocation-candidates.ps1is excluded by none and included by none
📒 Files selected for processing (30)
Assets/Tests/Editor/ThirdPartyToolMigrationFileServiceTests.csAssets/Tests/Editor/ThirdPartyToolMigrationRulesTests.csAssets/Tests/Editor/ThirdPartyToolMigrationWizardWindowTests.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationAliasRules.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationApiDetectionRules.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationAsmdefRules.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationAssemblyScopedNameMap.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationAssemblyUsageAnalyzer.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationAssemblyUsageAsyncAnalyzer.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationCSharpRules.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationDetectionRules.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationDomainDetectionRules.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationFastAssemblyRequirementCollector.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationFastSourceTargetDetector.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationModels.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationPlanBuilder.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationRuleCatalog.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationRules.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationScreenshotDetectionRules.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationScreenshotRules.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationTargetScanner.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationTypeReplacementRules.csPackages/src/Editor/Presentation/Setup/SetupWizardWindow.ussPackages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardText.csPackages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardView.csPackages/src/Editor/Presentation/Setup/ThirdPartyToolMigrationWizardWindow.csPackages/src/TemporarySkills~/v3-cli-invocation-migration/Skill/SKILL.mdPackages/src/TemporarySkills~/v3-cli-invocation-migration/Skill/references/first-party-v2-to-v3.mdPackages/src/TemporarySkills~/v3-cli-invocation-migration/Skill/scripts/detect-v3-cli-invocation-candidates.shcli/internal/cli/skills_test.go
💤 Files with no reviewable changes (1)
- Packages/src/TemporarySkills~/v3-cli-invocation-migration/Skill/scripts/detect-v3-cli-invocation-candidates.sh
There was a problem hiding this comment.
2 issues found across 31 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Only preserve bare FirstPartyTools capture references when a using directive is actually in scope. Fully qualified namespace usage can still trigger migration, but the migrated call is now qualified so the output remains compilable. Also reuse the existing alias-qualified matcher for current Domain contract detection instead of keeping a duplicate helper.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Use a neutral helper name because the same alias-qualified type check is shared by both legacy namespace aliases and current V3 namespace aliases.
Merged
This was referenced Jul 6, 2026
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
uloopinvocations.Changes
Verification
cli/dist/darwin-arm64/uloop compile --project-path "$(git rev-parse --show-toplevel)"cli/dist/darwin-arm64/uloop run-tests --project-path "$(git rev-parse --show-toplevel)" --test-mode EditMode --filter-type regex --filter-value ".*ThirdPartyToolMigration(RulesTests|FileServiceTests).*"before=False/0; applied=0; after=False/0; appliedFiles=autoreview --mode branch --base origin/v3-beta: clean