feat: V3 CLI invocation migration skills are available from the wizard#1382
Conversation
Add a temporary AI skill for migrating V2 uloop CLI invocations, expose dedicated CLI and migration-window install/remove paths, and cover first-party option and output-field detection without changing normal skill installs.
Clarify the wizard by separating mechanical C# source structure migration from the temporary AI skill used for SKILL.md and script updates.
Keep C# migration controls with the C# section, and move AI skill install controls outside the explanatory panel so the two migration paths read as distinct sections.
Align the C# and AI migration sections visually, add C# migration guidance, and separate the two paths with a divider so the wizard reads as two distinct workflows.
Use read-only selectable text fields for wizard copy while preserving the existing label-style presentation.
Show an optional prompt example after installing the migration skill so users know how to ask their AI agent to update SKILL.md and script invocations.
Apply the shared primary button style to the migration action so it matches the check and migration skill install actions.
Allow the V3 migration skill installer to locate Unity CLI Loop when the package is referenced through a manifest file dependency outside the project.
Recheck the installed migration skill files when the button is clicked so stale UI state cannot choose the wrong action. Remove the temporary V3 migration skill from both supported install layouts because older or alternate layouts should still be treated as installed.
Prune excluded directories before PowerShell detector recursion and read one-line files correctly. Restrict manifest-local package root resolution to the Unity CLI Loop package so unrelated local dependencies cannot shadow the temporary migration skill source.
Keep third-party local package skills discoverable through the general manifest path while using a filtered path only for resolving the Unity CLI Loop package root.
Skip the bundled temporary migration skill source when scanning and make the PowerShell output-field check case-sensitive so already migrated camelCase field reads are not reported again.
Treat punctuation and Markdown backticks as command terminators in both detectors so SKILL.md and Markdown examples are reported consistently.
Exclude TemporarySkills/v3-cli-invocation-migration regardless of the package parent layout so detector reruns do not report the bundled reference examples as self-noise.
Allow uninstall-v3-migration to remove the known temporary skill name even when the Unity CLI Loop package source cannot be resolved.
Make uninstall-v3-migration clear both flat and grouped install layouts so temporary migration skills do not remain after cleanup.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a ChangesV3 CLI Invocation Migration Skill
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
note over User,ToolSkillSynchronizer: Wizard skill target selection and async toggle
end
participant User
participant ThirdPartyToolMigrationWizardView
participant ThirdPartyToolMigrationWizardWindow
participant SkillSetupUseCase
participant ToolSkillSynchronizer
User->>ThirdPartyToolMigrationWizardView: Select SkillsTarget (EnumField)
ThirdPartyToolMigrationWizardView->>ThirdPartyToolMigrationWizardWindow: migrationSkillTargetChanged(target)
ThirdPartyToolMigrationWizardWindow->>SkillSetupUseCase: GetV3MigrationSkillInstallStateAtProjectRoot
SkillSetupUseCase->>ToolSkillSynchronizer: GetV3MigrationSkillInstallStateAtProjectRoot
ToolSkillSynchronizer-->>SkillSetupUseCase: SkillInstallState
SkillSetupUseCase-->>ThirdPartyToolMigrationWizardWindow: SkillInstallState
ThirdPartyToolMigrationWizardWindow->>ThirdPartyToolMigrationWizardView: UpdateMigrationSkillState(isUpdating=false)
User->>ThirdPartyToolMigrationWizardView: Click install/remove button
ThirdPartyToolMigrationWizardView->>ThirdPartyToolMigrationWizardWindow: toggleMigrationSkill()
ThirdPartyToolMigrationWizardWindow->>ThirdPartyToolMigrationWizardView: UpdateMigrationSkillState(isUpdating=true)
ThirdPartyToolMigrationWizardWindow->>SkillSetupUseCase: InstallV3MigrationSkillFilesAsync or RemoveV3MigrationSkillFilesAsync
SkillSetupUseCase->>ToolSkillSynchronizer: InstallV3MigrationSkillFilesAtProjectRoot or RemoveV3MigrationSkillFilesAtProjectRoot
ToolSkillSynchronizer-->>SkillSetupUseCase: SkillInstallResult
SkillSetupUseCase-->>ThirdPartyToolMigrationWizardWindow: complete
ThirdPartyToolMigrationWizardWindow->>SkillSetupUseCase: GetV3MigrationSkillInstallStateAtProjectRoot
SkillSetupUseCase-->>ThirdPartyToolMigrationWizardWindow: updated SkillInstallState
ThirdPartyToolMigrationWizardWindow->>ThirdPartyToolMigrationWizardView: UpdateMigrationSkillState(isUpdating=false)
Estimated code review effort🎯 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 |
Move the bundled migration skill source under a tilde-suffixed folder so Unity does not import it as package content. Update the CLI lookup path and detector self-exclusion to match the ignored folder name, and drop Unity meta files that are no longer needed for the ignored source tree.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Packages/src/TemporarySkills~/v3-cli-invocation-migration/Skill/scripts/detect-v3-cli-invocation-candidates.sh (1)
11-17: 💤 Low valueLine-by-line regex detection has expected limitations.
The boolean argument pattern correctly detects
--option=true/falseand--option true/falsesyntax, but by design it:
- Processes files line-by-line (won't detect multi-line commands with backslash continuations)
- May match inside code examples, comments, or string literals
- Uses greedy matching that spans multiple arguments
For a migration detection tool where candidates are reviewed by humans, these tradeoffs are acceptable. False positives are preferable to false negatives in this context.
🤖 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/scripts/detect-v3-cli-invocation-candidates.sh around lines 11 - 17, The regex pattern in the detect-v3-cli-invocation-candidates.sh script has documented limitations that are acceptable for this use case. Add comments in the awk script around the regex pattern (specifically the pattern that matches --option=true/false and --option true/false syntax) to document the known limitations: that it processes files line-by-line and won't detect multi-line commands with backslash continuations, may match inside code examples/comments/string literals, and uses greedy matching. Also document that these limitations are acceptable because the tool is designed for human review and false positives are preferable to false negatives in this migration detection context.
🤖 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/TemporarySkills`~/v3-cli-invocation-migration/Skill/scripts/detect-v3-cli-invocation-candidates.sh:
- Line 7: In the find command on line 7 within
detect-v3-cli-invocation-candidates.sh, there are two redundant patterns
excluding paths with TemporarySkills~/v3-cli-invocation-migration. The pattern
with the `*/` prefix (with wildcard) successfully matches and excludes the
directory, while the pattern without the `*/` prefix does not match due to how
find prepends directory paths. Remove the second, redundant `-path
'TemporarySkills~/v3-cli-invocation-migration'` pattern (the one without the
`*/` wildcard) from the prune section to simplify the find command while
maintaining the same exclusion behavior.
---
Nitpick comments:
In
`@Packages/src/TemporarySkills`~/v3-cli-invocation-migration/Skill/scripts/detect-v3-cli-invocation-candidates.sh:
- Around line 11-17: The regex pattern in the
detect-v3-cli-invocation-candidates.sh script has documented limitations that
are acceptable for this use case. Add comments in the awk script around the
regex pattern (specifically the pattern that matches --option=true/false and
--option true/false syntax) to document the known limitations: that it processes
files line-by-line and won't detect multi-line commands with backslash
continuations, may match inside code examples/comments/string literals, and uses
greedy matching. Also document that these limitations are acceptable because the
tool is designed for human review and false positives are preferable to false
negatives in this migration detection context.
🪄 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: b8ebc74a-7c06-4803-92bb-3e4f4155ff7c
⛔ 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 (6)
Packages/src/Editor/Domain/CliConstants.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_discovery.gocli/internal/cli/skills_test.go
💤 Files with no reviewable changes (2)
- Packages/src/TemporarySkills~/v3-cli-invocation-migration/Skill/SKILL.md
- Packages/src/TemporarySkills~/v3-cli-invocation-migration/Skill/references/first-party-v2-to-v3.md
🚧 Files skipped from review as they are similar to previous changes (3)
- Packages/src/Editor/Domain/CliConstants.cs
- cli/internal/cli/skills_discovery.go
- cli/internal/cli/skills_test.go
There was a problem hiding this comment.
3 issues found across 22 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Validate that the dedicated migration skill source declares the expected name before installation, avoid following PowerShell reparse point directories during scans, simplify the POSIX self-exclusion pattern, and make the POSIX detector test skip when sh is unavailable.
Read PowerShell detector inputs as UTF-8 so Windows PowerShell reports physical line numbers for UTF-8 no-BOM files. Normalize single-skill source paths before syncing so nested migration skill files are not deleted on Windows.
Document encoding, line ending, path separator, and shell validation rules so macOS-first changes are checked against Windows failure modes before they regress again.
d9a6d61 to
9f97ad6
Compare
Normalize the manifest package-root assertion before comparing paths so the Windows test accepts equivalent slash and backslash forms.
Summary
User Impact
Changes
Verification