chore: Split skill source discovery from layout handling

[hatayama]

Summary

• Split SKILL.md metadata parsing and skill source discovery out of SkillInstallLayout.
• Keep the existing SkillInstallLayout entry points intact as delegating wrappers so setup behavior remains unchanged.

User Impact

• No user-facing behavior change is intended.
• Skill setup maintenance is easier to review because layout/state logic is separated from source discovery and frontmatter parsing.

Changes

• Added SkillSourceFrontmatterReader for SKILL.md name, toolName, description, internal flag, and tool matching helpers.
• Added SkillSourceRootEnumerator for Assets / Packages / manifest file dependencies / PackageCache / CliOnlyTools~ source discovery.
• Kept SkillSourceInfo and layout constants on SkillInstallLayout, and left line-ending normalization in place for a later follow-up.
• Split the branch into two commits: frontmatter reader first, source discovery second.

Verification

• dist/darwin-arm64/uloop clear-console --project-path "/Users/a12115/ghq/hatayama/unity-cli-loop2"
• dist/darwin-arm64/uloop compile --project-path "/Users/a12115/ghq/hatayama/unity-cli-loop2" (0 errors, 0 warnings)
• dist/darwin-arm64/uloop run-tests --project-path "/Users/a12115/ghq/hatayama/unity-cli-loop2" --test-mode EditMode --filter-type regex --filter-value "^io.github.hatayama.UnityCliLoop.Tests.Editor.SkillInstallLayoutTests." (10 passed)
• dist/darwin-arm64/uloop run-tests --project-path "/Users/a12115/ghq/hatayama/unity-cli-loop2" --test-mode EditMode --filter-type regex --filter-value "^io.github.hatayama.UnityCliLoop.Tests.Editor.ToolSkillSynchronizerTests." (52 passed)
• dist/darwin-arm64/uloop run-tests --project-path "/Users/a12115/ghq/hatayama/unity-cli-loop2" --test-mode EditMode --filter-type regex --filter-value "^io.github.hatayama.UnityCliLoop.Tests.Editor.StaticFacadeStateGuardTests." (20 passed)
• git diff --check origin/v3-beta...HEAD

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR refactors SkillInstallLayout.cs to delegate skill metadata parsing and root enumeration to two new internal helper classes: SkillSourceFrontmatterReader (frontmatter parsing utilities) and SkillSourceRootEnumerator (skill source root discovery and enumeration). Two methods change accessibility from private to internal.

Changes

Skill source refactor

Layer / File(s)	Summary
Frontmatter parsing helper Packages/src/Editor/Infrastructure/SkillSetup/SkillSourceFrontmatterReader.cs	New static class with methods to parse toolName, name, description from SKILL.md frontmatter, resolve tool names, match skill content to a tool, and detect internal skills.
Skill source root enumerator Packages/src/Editor/Infrastructure/SkillSetup/SkillSourceRootEnumerator.cs	New class discovering skill roots (CLI-only tools, Assets, Packages, manifest dependencies, PackageCache), enumerating SKILL.md files, and exposing GetInternalSkillToolNames, GetSkillSourceInfos, GetToolDescriptionsByToolName.
SkillInstallLayout delegation and cleanup Packages/src/Editor/Infrastructure/SkillSetup/SkillInstallLayout.cs	Replaces in-file regex parsing and enumeration logic with calls to the new helper classes; removes unused constants, types, and the System.Text.RegularExpressions import; changes CollectSourceSkillFiles and IsSafeSkillPathComponent to internal.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

• hatayama/unity-cli-loop#951: Introduced the underlying skill install layout logic parsing SKILL.md metadata and comparing expected vs installed files, now refactored to use the new helpers.
• hatayama/unity-cli-loop#980: Changed GetInstalledState/installed-skill detection logic in SkillInstallLayout that overlaps with this refactor.
• hatayama/unity-cli-loop#1588: Added tests for SkillInstallLayout behaviors like GetSkillSourceInfos and tool-description parsing, areas refactored in this PR.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main refactor: splitting skill source discovery from layout handling.
Description check	✅ Passed	The description directly matches the refactor and its verification details.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feature/hatayama/split-skill-source-discovery

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 3

🤖 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/SkillSetup/SkillSourceFrontmatterReader.cs`:
- Around line 23-29: `SkillSourceFrontmatterReader` is parsing quoted
frontmatter inconsistently: `ReadToolName` and the `internal` handling should
normalize quoted scalar values the same way `name` and `description` do. Update
the parsing logic so `toolName` and `internal` strip surrounding quotes before
comparison/boolean interpretation, and reuse the existing frontmatter value
normalization path or equivalent helper in `SkillSourceFrontmatterReader` to
keep behavior consistent across all fields.

In `@Packages/src/Editor/Infrastructure/SkillSetup/SkillSourceRootEnumerator.cs`:
- Around line 260-262: The dependency name set in SkillSourceRootEnumerator
includes local file/path dependencies, which can later be matched again against
PackageCache and add duplicate stale skills. Update the dependency collection
around EnumerateManifestDependencies so local dependencies are filtered out
before building dependencyNames, and keep PackageCache matching limited to
non-local manifest dependencies only.
- Around line 168-202: `EnumerateSkillSourceRoots` is losing the intended
precedence because it yields from a `HashSet<string>`, whose enumeration order
is not the explicit CLI-only → Assets → Packages → manifest → cache order
required by `GetSkillSources`. Update the `EnumerateSkillSourceRoots` flow to
preserve insertion order while still deduplicating roots, and keep
`AddSkillSourceRoot` responsible only for normalization and duplicate checks so
the first matching `skillName` remains deterministic.

🪄 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: 6de3c577-b9d7-4867-9533-d3a28d67b2ff

📥 Commits

Reviewing files that changed from the base of the PR and between d3388a4 and be672a9.

⛔ Files ignored due to path filters (2)

• Packages/src/Editor/Infrastructure/SkillSetup/SkillSourceFrontmatterReader.cs.meta is excluded by none and included by none
• Packages/src/Editor/Infrastructure/SkillSetup/SkillSourceRootEnumerator.cs.meta is excluded by none and included by none

📒 Files selected for processing (3)

• Packages/src/Editor/Infrastructure/SkillSetup/SkillInstallLayout.cs
• Packages/src/Editor/Infrastructure/SkillSetup/SkillSourceFrontmatterReader.cs
• Packages/src/Editor/Infrastructure/SkillSetup/SkillSourceRootEnumerator.cs

[cubic-dev-ai]

5 issues found across 5 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>