Repository navigation
chore: Simplify skill setup synchronization internals - #1588
Conversation
Drop test-only target detection wrappers so the synchronizer has fewer entry points before class extraction. Update tests to call the explicit project-root overload that preserves the existing freshness-check behavior.
Drop public install wrappers that were only exercised by tests so the synchronizer keeps one explicit install entry point for the setup service. Update tests to pass detected targets and disabled tool inputs directly.
Require callers to pass disabled tool inputs explicitly when installing skill files at a project root. This removes another set of test-only forwarding methods before splitting the synchronizer responsibilities.
Rename internal detection and per-tool install methods so their names describe layout scope and disabled-tool inputs. This removes the remaining overload pressure in ToolSkillSynchronizer without changing public setup APIs.
Move atomic skill file writes, rollback capture, stale file cleanup, and excluded-file checks into SkillDirectoryContentSynchronizer. This isolates the file-content synchronization responsibility before splitting target detection and installation flow.
Move target definitions and install-state detection into SkillTargetDetector. Keep ToolSkillSynchronizer as the setup-facing facade while tests exercise the detector directly.
Move skill-to-tool resolution and disabled-tool checks into SkillDisabledToolFilter. This separates settings-based filtering from the remaining installation orchestration.
Move target-root install, cleanup, deprecated-skill removal, and managed directory pruning into SkillTargetInstaller. This leaves ToolSkillSynchronizer focused on orchestration and result aggregation.
Move temporary v3 migration skill detection, install, removal, and specific-skill helpers into V3MigrationSkillInstaller. Keep ToolSkillSynchronizer as the service-facing facade for migration operations.
Keep the public nested SkillInstallResult and SkillTargetInfo API names unchanged while moving their definitions out of the facade file. This brings ToolSkillSynchronizer under the target size after responsibility extraction.
Add CancellationToken parameters to the remaining skill setup async paths so service-level cancellation reaches Task.Run boundaries. Update direct tests to pass CancellationToken.None through the refactored helpers.
Separate layout and catalog behavior tests from ToolSkillSynchronizerTests so the synchronizer fixture now focuses on synchronization responsibilities after the class split.
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR refactors ChangesSkill Install Pipeline Refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
Assets/Tests/Editor/ToolSkillSynchronizerTests.cs (1)
1655-1681: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated test helper drifting from
SkillInstallLayoutTests.cscounterpart.
CreateFakeSourceSkillhere has grown asourceMetaFileRelativePathparameter that the near-identical helper inSkillInstallLayoutTests.cs(Lines 317-338) lacks. Both files also duplicateWriteSkillFileandCreateTemporaryProjectRoot. Consider extracting a shared test fixture/helper class (e.g., underAssets/Tests/Editor/) now that the layout tests were split out, to avoid silent divergence between the two suites going forward.🤖 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 `@Assets/Tests/Editor/ToolSkillSynchronizerTests.cs` around lines 1655 - 1681, The test helper `CreateFakeSourceSkill` has diverged from its duplicate in `SkillInstallLayoutTests.cs`, which risks the two suites drifting apart. Update the shared test support by extracting the duplicated helpers (`CreateFakeSourceSkill`, `WriteSkillFile`, and `CreateTemporaryProjectRoot`) into a common editor test utility/fixture under `Assets/Tests/Editor/`, then have both `ToolSkillSynchronizerTests` and `SkillInstallLayoutTests` use that shared implementation so the new `sourceMetaFileRelativePath` behavior stays consistent.Packages/src/Editor/Infrastructure/SkillSetup/SkillTargetDetector.cs (2)
128-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing precondition assert for consistency.
DetectTargetsAcrossLayoutsAtProjectRoot(line 46) andDetectTargetsForLayoutStateAtProjectRoot(line 86) both assert!string.IsNullOrEmpty(projectRoot), butDetectTargetsWithSkillsDirectoryskips this check. If called with an empty/nullprojectRoot, it will silently combine with an empty base path instead of failing fast during development.🛡️ Proposed fix
internal static List<ToolSkillSynchronizer.SkillTargetInfo> DetectTargetsWithSkillsDirectory(string projectRoot) { + Debug.Assert(!string.IsNullOrEmpty(projectRoot), "projectRoot must not be null or empty"); + List<ToolSkillSynchronizer.SkillTargetInfo> targets = new();Based on learnings, this repo follows a contract-programming convention using
Debug.Assertto enforce preconditions and fail fast on programmer errors.🤖 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/Infrastructure/SkillSetup/SkillTargetDetector.cs` around lines 128 - 134, Add the missing precondition assert in DetectTargetsWithSkillsDirectory to match the contract-programming pattern used by DetectTargetsAcrossLayoutsAtProjectRoot and DetectTargetsForLayoutStateAtProjectRoot. Before using projectRoot in the Path.Combine logic, assert that it is not null or empty with Debug.Assert so the method fails fast on invalid input instead of silently proceeding with an empty base path.Source: Learnings
42-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove
DetectTargetsAcrossLayoutsAtProjectRootif it’s no longer used. No call sites remain inPackages/src, so this looks like dead code after the refactor.🤖 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/Infrastructure/SkillSetup/SkillTargetDetector.cs` around lines 42 - 78, Remove the now-unused DetectTargetsAcrossLayoutsAtProjectRoot method from SkillTargetDetector, since there are no remaining call sites in Packages/src after the refactor. Verify any related references in ToolSkillSynchronizer or SkillInstallLayout are not relying on it, and delete the dead code rather than keeping an unused helper around.
🤖 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.
Nitpick comments:
In `@Assets/Tests/Editor/ToolSkillSynchronizerTests.cs`:
- Around line 1655-1681: The test helper `CreateFakeSourceSkill` has diverged
from its duplicate in `SkillInstallLayoutTests.cs`, which risks the two suites
drifting apart. Update the shared test support by extracting the duplicated
helpers (`CreateFakeSourceSkill`, `WriteSkillFile`, and
`CreateTemporaryProjectRoot`) into a common editor test utility/fixture under
`Assets/Tests/Editor/`, then have both `ToolSkillSynchronizerTests` and
`SkillInstallLayoutTests` use that shared implementation so the new
`sourceMetaFileRelativePath` behavior stays consistent.
In `@Packages/src/Editor/Infrastructure/SkillSetup/SkillTargetDetector.cs`:
- Around line 128-134: Add the missing precondition assert in
DetectTargetsWithSkillsDirectory to match the contract-programming pattern used
by DetectTargetsAcrossLayoutsAtProjectRoot and
DetectTargetsForLayoutStateAtProjectRoot. Before using projectRoot in the
Path.Combine logic, assert that it is not null or empty with Debug.Assert so the
method fails fast on invalid input instead of silently proceeding with an empty
base path.
- Around line 42-78: Remove the now-unused
DetectTargetsAcrossLayoutsAtProjectRoot method from SkillTargetDetector, since
there are no remaining call sites in Packages/src after the refactor. Verify any
related references in ToolSkillSynchronizer or SkillInstallLayout are not
relying on it, and delete the dead code rather than keeping an unused helper
around.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0f99c3c2-1765-4f25-8ba6-c717ee2a6962
⛔ Files ignored due to path filters (7)
Assets/Tests/Editor/SkillInstallLayoutTests.cs.metais excluded by none and included by nonePackages/src/Editor/Infrastructure/SkillSetup/SkillDirectoryContentSynchronizer.cs.metais excluded by none and included by nonePackages/src/Editor/Infrastructure/SkillSetup/SkillDisabledToolFilter.cs.metais excluded by none and included by nonePackages/src/Editor/Infrastructure/SkillSetup/SkillTargetDetector.cs.metais excluded by none and included by nonePackages/src/Editor/Infrastructure/SkillSetup/SkillTargetInstaller.cs.metais excluded by none and included by nonePackages/src/Editor/Infrastructure/SkillSetup/ToolSkillSynchronizer.Types.cs.metais excluded by none and included by nonePackages/src/Editor/Infrastructure/SkillSetup/V3MigrationSkillInstaller.cs.metais excluded by none and included by none
📒 Files selected for processing (10)
Assets/Tests/Editor/SkillInstallLayoutTests.csAssets/Tests/Editor/ToolSkillSynchronizerTests.csPackages/src/Editor/Infrastructure/SkillSetup/SkillDirectoryContentSynchronizer.csPackages/src/Editor/Infrastructure/SkillSetup/SkillDisabledToolFilter.csPackages/src/Editor/Infrastructure/SkillSetup/SkillTargetDetector.csPackages/src/Editor/Infrastructure/SkillSetup/SkillTargetInstaller.csPackages/src/Editor/Infrastructure/SkillSetup/ToolSkillSetupService.csPackages/src/Editor/Infrastructure/SkillSetup/ToolSkillSynchronizer.Types.csPackages/src/Editor/Infrastructure/SkillSetup/ToolSkillSynchronizer.csPackages/src/Editor/Infrastructure/SkillSetup/V3MigrationSkillInstaller.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 17 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Delete GetCurrentDisabledTools after the synchronizer facade no longer calls it, keeping the extracted filter free of unreachable wrapper code.
Poll the synchronization CancellationToken inside target and file loops so a started sync can stop before continuing writes or deletes. Also fail fast when the packaged V3 migration skill source declares an unexpected skill name.
- The skill target example named Cursor, which was removed as a target in #1983; use Codex CLI, a target that actually exists. - The naming-debt list cited ToolSkillSynchronizer.SkillTargetDefinition, which no longer exists: #1588 moved it into SkillTargetDetector as a private struct, so it is not a public identifier kept by policy. - The complexity threshold is declared in three places, not two: the code-complexity workflow's artifact step hardcodes --max-complexity 15 and bypasses check-code-complexity.sh entirely.
1 participant
Summary
User Impact
Changes
SkillDirectoryContentSynchronizer,SkillTargetDetector,SkillDisabledToolFilter,SkillTargetInstaller, andV3MigrationSkillInstaller.CancellationToken ctparameters and start-of-operation cancellation checks for surviving async skill setup methods; loop-internal cancellation remains out of scope for this refactor.Verification
dist/darwin-arm64/uloop compile --project-path "$(git rev-parse --show-toplevel)"dist/darwin-arm64/uloop run-tests --project-path "$(git rev-parse --show-toplevel)" --test-mode EditMode --filter-type regex --filter-value '.*(ToolSkillSynchronizerTests|SkillInstallLayoutTests).*'