Repository navigation
chore: Unify skill source discovery - #1486
Conversation
Route skill package discovery through PackageIdentity and PackageSearchResult so source roots are enumerated from one skillscan entry point. Remove the clicore re-export wrappers and point dispatcher skill discovery directly at common/skillscan while preserving the existing Unity package root priority.
|
Warning Review limit reached
Next review available in: 1 minute 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 (4)
📝 WalkthroughWalkthroughThe clicore package's skillscan alias file is deleted, and all consumers (clicore's tool_catalog, dispatcher's skills/skills_discovery/skills_sync) now reference common/skillscan directly. The skillscan package's package-root discovery is rewritten around new PackageIdentity/PackageSearchResult types with revised ranking logic. Stamp hashes are updated. ChangesSkillscan migration and package discovery rework
Estimated code review effort: 3 (Moderate) | ~25 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 (1)
cli/common/skillscan/packages.go (1)
142-218: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant manifest/cache reads within a single discovery pass.
enumerateManifestLocalPackageResults(Line 143) andenumeratePackageCacheResults(Line 164) each independently callreadManifestDependencies(projectRoot), parsingPackages/manifest.jsontwice perEnumeratePackageSearchResultscall. Likewise,enumeratePackageCacheResults(Line 173) andenumerateUnityCliLoopPackageCacheFallbackResults(Line 201) both callos.ReadDiron the sameLibrary/PackageCachedirectory. SinceEnumeratePackageSearchResultschains all four helpers on every discovery pass (invoked from bothResolvePackageRoot/FindUnityCliLoopPackageandEnumerateSourceRoots), this doubles the file/dir I/O unnecessarily.Consider reading the manifest and cache directory listing once and threading the results into the relevant helpers.
♻️ Sketch of the refactor
-func enumerateManifestLocalPackageResults(projectRoot string) []PackageSearchResult { - dependencies := readManifestDependencies(projectRoot) +func enumerateManifestLocalPackageResults(projectRoot string, dependencies map[string]string) []PackageSearchResult { if len(dependencies) == 0 { return []PackageSearchResult{} } ... } -func enumeratePackageCacheResults(projectRoot string) []PackageSearchResult { - dependencies := readManifestDependencies(projectRoot) +func enumeratePackageCacheResults(projectRoot string, dependencies map[string]string) []PackageSearchResult { if len(dependencies) == 0 { return []PackageSearchResult{} } ... - entries, err := os.ReadDir(packageCacheDir) + entries, err := readPackageCacheEntries(projectRoot) // shared, memoized read ... }🤖 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 `@cli/common/skillscan/packages.go` around lines 142 - 218, `EnumeratePackageSearchResults` currently triggers duplicate I/O by letting both `enumerateManifestLocalPackageResults` and `enumeratePackageCacheResults` call `readManifestDependencies`, and both `enumeratePackageCacheResults` and `enumerateUnityCliLoopPackageCacheFallbackResults` call `os.ReadDir` on the same cache directory. Refactor the discovery flow so the manifest dependencies and PackageCache entries are read once per pass, then pass those shared results into the affected helpers (`enumerateManifestLocalPackageResults`, `enumeratePackageCacheResults`, and `enumerateUnityCliLoopPackageCacheFallbackResults`) instead of re-reading them internally.
🤖 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 `@cli/common/skillscan/packages.go`:
- Around line 142-218: `EnumeratePackageSearchResults` currently triggers
duplicate I/O by letting both `enumerateManifestLocalPackageResults` and
`enumeratePackageCacheResults` call `readManifestDependencies`, and both
`enumeratePackageCacheResults` and
`enumerateUnityCliLoopPackageCacheFallbackResults` call `os.ReadDir` on the same
cache directory. Refactor the discovery flow so the manifest dependencies and
PackageCache entries are read once per pass, then pass those shared results into
the affected helpers (`enumerateManifestLocalPackageResults`,
`enumeratePackageCacheResults`, and
`enumerateUnityCliLoopPackageCacheFallbackResults`) instead of re-reading them
internally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 06ca2b90-1056-4c46-9b99-be075cf2b3bd
📒 Files selected for processing (11)
cli/common/clicore/skillscan_aliases.gocli/common/clicore/tool_catalog.gocli/common/clicore/tool_catalog_test.gocli/common/skillscan/packages.gocli/common/skillscan/packages_test.gocli/common/skillscan/sources.gocli/dispatcher/internal/dispatcher/skills.gocli/dispatcher/internal/dispatcher/skills_discovery.gocli/dispatcher/internal/dispatcher/skills_sync.gocli/dispatcher/shared-inputs-stamp.jsoncli/project-runner/shared-inputs-stamp.json
💤 Files with no reviewable changes (1)
- cli/common/clicore/skillscan_aliases.go
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 11 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Keep the legacy marker-only fallback scoped to Packages/src so unrelated project packages with FirstPartyTools markers are not resolved as the Unity CLI Loop package.
1 participant
Summary
User Impact
Changes
Verification