feat: Skills use the project-matched CLI by default - #1404
Merged
Conversation
Allow project skill installs to opt into npx-based uloop-cli invocation while keeping global invocation as the default. Store the choice in tool settings, apply it consistently in CLI and Unity-driven installs/status checks, and expose the toggle in setup/settings UI.
Move the project CLI version toggle out of the skills controls so it reads as a CLI execution mode. Hide global CLI install/status UI while project CLI mode is enabled, while keeping global CLI as the default path.
Handle clicks on the checkbox area even when Unity's Toggle control does not emit a value change, while avoiding double toggles when the built-in control already handled the click.
Project CLI version installs must keep every markdown workflow on the pinned package CLI. Apply the npx rewrite to referenced markdown files as well as SKILL.md in both CLI and Editor installers.
Setup Wizard should let users choose where to install skills when no target tool folders are present, instead of blocking installation behind a create-folder message.
Make project skill installation use the package-matched npx command by default so new installs avoid global CLI version drift, while preserving explicit global mode as an opt-out.
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 (6)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds per-project skill CLI invocation state, ChangesProject CLI invocation
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 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 |
There was a problem hiding this comment.
2 issues found across 21 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
Packages/src/Editor/UI/UIToolkit/Components/CliSetupSection.cs (2)
320-337: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRow-click dedup strategy differs from
HandleGroupSkillsRowClicked.The group-skills row guards against double-invocation by checking whether the click target is inside the toggle, while this handler relies on the
_projectCliVersionChangedByToggleflag plus a scheduled reset. Both are functionally correct, but aligning the two row handlers on one approach would reduce cognitive load. Optional.🤖 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/UI/UIToolkit/Components/CliSetupSection.cs` around lines 320 - 337, Align HandleProjectCliVersionRowClicked with the row-click dedup pattern used in HandleGroupSkillsRowClicked by using the same click-target-inside-toggle guard instead of relying on _projectCliVersionChangedByToggle and a scheduled reset. Update the CliSetupSection row handler to follow one consistent approach for both row-click methods, keeping the toggle update and OnUseProjectCliVersionChanged invocation behavior unchanged.
233-243: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMisleading
isCliInstalledparameter name.
UpdateInstallSkillsButtonnow passesCanManageSkills(data)(true when either the global CLI is installed or project CLI/npx mode is enabled) into theisCliInstalledparameter ofGetInstallSkillsButtonTextandIsInstallSkillsButtonEnabled. The behavior is correct, but the parameter name no longer reflects what is passed. Consider renaming tocanManageSkillsfor clarity.Also applies to: 259-300
🤖 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/UI/UIToolkit/Components/CliSetupSection.cs` around lines 233 - 243, The `UpdateInstallSkillsButton` path is passing `CanManageSkills(data)` into parameters named `isCliInstalled`, which is misleading because the value now means “can manage skills” rather than strict CLI installation. Rename the relevant parameter(s) in `GetInstallSkillsButtonText` and `IsInstallSkillsButtonEnabled` to `canManageSkills` (and update their call sites/usages in the related methods around `UpdateInstallSkillsButton`) so the naming matches the behavior.
🤖 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/Cli`~/src/skills/skills-manager.ts:
- Around line 886-899: The crash comes from collectAllSkills passing a null
packageVersion into formatSkillsForCliInvocation for local invocations. Update
collectAllSkills so that when cliInvocation is not global and
resolvePackageRoot(projectRoot) returns null, it handles that case before
calling formatSkillsForCliInvocation, either by returning a safe empty result or
by throwing a clear specific error. Keep the logic centered around
collectAllSkills, resolvePackageRoot, and formatSkillsForCliInvocation so local
npx runs do not hit the non-null assertion in formatSkillsForCliInvocation.
In `@Packages/src/Cli`~/src/tool-settings-loader.ts:
- Around line 137-141: Reject arrays in the tool settings parser so they cannot
be returned as ToolSettingsData and later serialize to an empty array. Update
the validation in tool-settings-loader.ts around the parsed object check to
explicitly exclude array values in addition to null/non-objects, so only plain
object settings are accepted before the parsed value is cast and persisted.
In `@Packages/src/Editor/UI/Setup/SetupWizardWindow.cs`:
- Around line 345-356: The click guard in InitializeProjectCliVersionToggle is
relying on _projectCliVersionChangedByToggle being set too late, so update the
row click handling to match HandleGroupSkillsRowClicked by inspecting the
ClickEvent source directly instead of using the async flag. In
SetupWizardWindow, change the _projectCliVersionRow.RegisterCallback<ClickEvent>
path so HandleProjectCliVersionRowClicked is skipped when the event originated
from _projectCliVersionToggle (or its toggle/label subtree), and keep the
toggle’s value-change callback only for handling actual value updates.
---
Nitpick comments:
In `@Packages/src/Editor/UI/UIToolkit/Components/CliSetupSection.cs`:
- Around line 320-337: Align HandleProjectCliVersionRowClicked with the
row-click dedup pattern used in HandleGroupSkillsRowClicked by using the same
click-target-inside-toggle guard instead of relying on
_projectCliVersionChangedByToggle and a scheduled reset. Update the
CliSetupSection row handler to follow one consistent approach for both row-click
methods, keeping the toggle update and OnUseProjectCliVersionChanged invocation
behavior unchanged.
- Around line 233-243: The `UpdateInstallSkillsButton` path is passing
`CanManageSkills(data)` into parameters named `isCliInstalled`, which is
misleading because the value now means “can manage skills” rather than strict
CLI installation. Rename the relevant parameter(s) in
`GetInstallSkillsButtonText` and `IsInstallSkillsButtonEnabled` to
`canManageSkills` (and update their call sites/usages in the related methods
around `UpdateInstallSkillsButton`) so the naming matches the behavior.
🪄 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: 26ce8fb7-7c93-463c-b2cf-eacc686740f9
📒 Files selected for processing (21)
Assets/Tests/Editor/SetupWizardWindowTests.csAssets/Tests/Editor/SkillsTargetSelectionResolverTests.csAssets/Tests/Editor/ToolSettingsTests.csAssets/Tests/Editor/ToolSkillSynchronizerTests.csPackages/src/Cli~/src/__tests__/skills-command.test.tsPackages/src/Cli~/src/__tests__/skills-manager.test.tsPackages/src/Cli~/src/__tests__/tool-settings-loader.test.tsPackages/src/Cli~/src/skills/skills-command.tsPackages/src/Cli~/src/skills/skills-manager.tsPackages/src/Cli~/src/tool-settings-loader.tsPackages/src/Editor/CLI/CliConstants.csPackages/src/Editor/Config/SkillInstallLayout.csPackages/src/Editor/Config/ToolSettings.csPackages/src/Editor/Config/ToolSettingsData.csPackages/src/Editor/UI/McpEditorWindow.csPackages/src/Editor/UI/McpEditorWindowViewData.csPackages/src/Editor/UI/Setup/SetupWizardWindow.csPackages/src/Editor/UI/Setup/SetupWizardWindow.uxmlPackages/src/Editor/UI/UIToolkit/Components/CliSetupSection.csPackages/src/Editor/UI/UIToolkit/McpEditorWindow.uxmlPackages/src/Editor/UI/UIToolkit/McpEditorWindowUI.cs
Avoid internal assertions when package metadata is unavailable for project CLI mode, reject array-shaped tool settings before saving invocation preferences, and make project CLI row clicks ignore events that originate from the toggle itself.
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
npx.uloopcommand from the setup/settings UI.User Impact
Changes
globalandnpxmodes.npx --yes uloop-cli@<packageVersion>in project CLI mode.Verification
uloop compileuloop run-tests --test-mode EditMode --filter-type regex --filter-value ".*ToolSettingsTests.*|.*ToolSkillSynchronizerTests.*SkillCliInvocation.*"npm run test:unit -- --runTestsByPath src/__tests__/tool-settings-loader.test.ts src/__tests__/skills-manager.test.ts src/__tests__/skills-command.test.ts --runInBandnpm run lintnpm run buildCloses #1385
This PR adds project-scoped CLI invocation support for skills, defaulting to
npx-based execution so generated/installed skill markdown invokes the Unity project’s pinneduloop-cliversion rather than the globally installed binary.Key changes:
globalvsnpxskill CLI invocation mode persisted in.uloop/settings.tools.jsonvia newskillCliInvocationsetting, with normalization and safe defaults (defaulting tonpxwhen settings are missing/invalid/unreadable).uloopinvocations becomenpx --yes uloop-cli@<packageVersion>, using the project’s installed Unity CLI Loop packagepackage.jsonversion(and bypassing rewriting whenglobalis selected).IsCliInstalled || UseProjectCliVersion) for managing skills.npxback toglobalmarks previouslynpx-installed skills as outdated.--cli-invocation npx|global, validating inputs and rejecting conflicting--cli-invocationusage with--global.Tests were added/updated to cover:
skillCliInvocation.npxmode and outdated detection when toggling back toglobal.--cli-invocation, plus project CLI version resolution and install failure/error cases when the Unity CLI Loop package root/version can’t be resolved.