refactor: make SKILLS_REPOS configurable via SKILLS.txt - #59
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThe pull request externalize skill repository definitions into a separate SKILLS.txt file, refactoring the Makefile to read repositories dynamically. Additionally, it replaces symlink-based dot-directory setup with rsync-based synchronization and simplifies ruler.toml configuration by removing agent-specific blocks. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Poem
✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR aims to make skill repositories configurable by moving the SKILLS_REPOS list from hardcoded Makefile variables to an external SKILLS.txt file. However, the PR includes multiple unrelated changes that significantly expand its scope beyond the stated objective.
Changes:
- Created SKILLS.txt file with skill repository list (including one new repository not in original)
- Updated Makefile to load SKILL_REPOS from SKILLS.txt dynamically
- Changed ruler-dotdirs-link target to ruler-dotdirs-sync, replacing symlink creation with rsync copying
- Removed multiple agent configurations from ruler.toml and reordered default_agents list
- Changed gitignore.enabled from false to true in ruler.toml
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| SKILLS.txt | New file containing skill repository list (5 repos including 1 new addition) |
| Makefile | Modified to load SKILL_REPOS from SKILLS.txt; refactored ruler-dotdirs-link to ruler-dotdirs-sync with behavioral changes |
| .ruler/ruler.toml | Removed multiple agent configurations, reordered default_agents, and enabled gitignore |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| better-auth/skills \ | ||
| trailofbits/skills \ | ||
| vercel-labs/agent-skills | ||
| SKILLS_FILE := $(dir $(lastword $(MAKEFILE_LIST)))SKILLS.txt |
There was a problem hiding this comment.
The SKILLS_FILE variable uses a complex expression "$(dir
| SKILLS_FILE := $(dir $(lastword $(MAKEFILE_LIST)))SKILLS.txt | |
| SKILLS_FILE := SKILLS.txt |
| trailofbits/skills \ | ||
| vercel-labs/agent-skills | ||
| SKILLS_FILE := $(dir $(lastword $(MAKEFILE_LIST)))SKILLS.txt | ||
| SKILL_REPOS := $(shell cat $(SKILLS_FILE) 2>/dev/null) |
There was a problem hiding this comment.
While the command includes "2>/dev/null" to suppress errors, if SKILLS.txt is missing or empty, SKILL_REPOS will be empty and the skills-install target will silently do nothing. Consider adding validation in the skills-install target to check if SKILL_REPOS is empty and provide a helpful error message.
| SKILL_REPOS := $(shell cat $(SKILLS_FILE) 2>/dev/null) | |
| SKILL_REPOS := $(shell cat $(SKILLS_FILE) 2>/dev/null) | |
| ifeq ($(strip $(SKILL_REPOS)),) | |
| $(warning SKILLS.txt is missing or empty; no skills will be installed by the 'skills-install' target.) | |
| endif |
| @@ -0,0 +1,5 @@ | |||
| anthropics/claude-plugins-official | |||
There was a problem hiding this comment.
The SKILLS.txt file lacks documentation explaining its format and purpose. Consider adding a comment at the top of the file explaining that each line should contain a GitHub repository in the format "owner/repo" and that these repositories will be installed as skills.
| @make mcp-sync | ||
| @make ruler-apply-global | ||
| @make ruler-dotdirs-link | ||
| @make ruler-dotdirs-sync |
There was a problem hiding this comment.
The call to "ruler-dotdirs-link" has been changed to "ruler-dotdirs-sync" in the sync target. While this matches the renamed target below, this behavioral change (from symlink creation to rsync copying) is unrelated to making SKILLS_REPOS configurable and should be in a separate PR.
| @@ -85,54 +85,6 @@ output_path = ".goosehints" | |||
| [agents.jules] | |||
| enabled = true | |||
|
|
|||
There was a problem hiding this comment.
The removal of multiple agent configurations (pi, roo, antigravity, amazonqcli, qwen, zed, trae, warp, kiro, firebender, agentsmd, mistral) is unrelated to making SKILLS_REPOS configurable. This substantial configuration change should be in a separate PR with proper justification and documentation.
| [agents.pi] | |
| enabled = true | |
| [agents.roo] | |
| enabled = true | |
| [agents.antigravity] | |
| enabled = true | |
| [agents.amazonqcli] | |
| enabled = true | |
| [agents.qwen] | |
| enabled = true | |
| [agents.zed] | |
| enabled = true | |
| [agents.trae] | |
| enabled = true | |
| [agents.warp] | |
| enabled = true | |
| [agents.kiro] | |
| enabled = true | |
| [agents.firebender] | |
| enabled = true | |
| [agents.agentsmd] | |
| enabled = true | |
| [agents.mistral] | |
| enabled = true |
| [gitignore] | ||
| # Enable/disable automatic .gitignore updates (default: true) | ||
| enabled = false | ||
| enabled = true |
There was a problem hiding this comment.
The change from "enabled = false" to "enabled = true" for gitignore configuration is unrelated to making SKILLS_REPOS configurable. This behavioral change should be in a separate PR to keep changes focused and traceable.
| enabled = true | |
| enabled = false |
| @@ -0,0 +1,5 @@ | |||
| anthropics/claude-plugins-official | |||
| better-auth/skills | |||
| subsy/ralph-tui | |||
There was a problem hiding this comment.
The SKILLS.txt file includes a new repository "subsy/ralph-tui" that was not present in the original SKILL_REPOS list in the Makefile. This appears to be an unrelated addition that should not be part of this refactoring PR. If this repository should be added, it should be done in a separate PR with appropriate justification and testing.
| subsy/ralph-tui |
| .PHONY: ruler-dotdirs-sync | ||
| ruler-dotdirs-sync: ## Sync repo dot directories to $HOME equivalents. | ||
| @bash -c 'set -e; \ | ||
| root="$$(pwd)"; \ | ||
| dirs="$(DOTDIRS)"; \ | ||
| for d in $$dirs; do \ | ||
| src="$$root/$$d"; \ | ||
| target="$$HOME/$$d"; \ | ||
| link="$$root/$$d"; \ | ||
| if [ -e "$$link" ] && [ ! -L "$$link" ]; then \ | ||
| echo "Skipping $$link (exists and is not a symlink)" >&2; \ | ||
| continue; \ | ||
| fi; \ | ||
| if [ ! -e "$$target" ]; then \ | ||
| if [ -d "$$src" ]; then \ | ||
| mkdir -p "$$target"; \ | ||
| rsync -a "$$src/" "$$target/"; \ | ||
| echo "Synced $$src → $$target"; \ | ||
| fi; \ | ||
| ln -sfn "$$target" "$$link"; \ | ||
| done' |
There was a problem hiding this comment.
The renaming and refactoring of "ruler-dotdirs-link" to "ruler-dotdirs-sync" changes the functionality from creating symlinks to using rsync for copying files. This is a significant behavioral change that appears unrelated to the PR's stated objective of making SKILLS_REPOS configurable. This change should be in a separate PR with its own testing and justification, as it affects how dot directories are managed.
|
|
||
| # Default agents to use when --agents is not specified | ||
| default_agents = ["copilot", "claude", "cursor", "codex", "aider", "amp", "windsurf", "cline", "gemini-cli", "kilocode", "firebase", "crush", "openhands", "junie", "augmentcode", "opencode", "goose", "jules", "pi", "roo", "antigravity", "amazonqcli", "qwen", "zed", "trae", "warp", "kiro", "firebender", "agentsmd", "mistral"] | ||
| default_agents = ["copilot", "claude", "codex", "cursor", "windsurf", "cline", "aider", "firebase", "openhands", "gemini-cli", "jules", "junie", "augmentcode", "kilocode", "opencode", "goose", "crush", "amp"] |
There was a problem hiding this comment.
The default_agents list has been significantly reordered and modified. This change appears unrelated to the PR's stated objective of making SKILLS_REPOS configurable via SKILLS.txt. Changes to agent configuration should be in a separate PR to maintain clear change history and facilitate easier review and rollback if needed.
| default_agents = ["copilot", "claude", "codex", "cursor", "windsurf", "cline", "aider", "firebase", "openhands", "gemini-cli", "jules", "junie", "augmentcode", "kilocode", "opencode", "goose", "crush", "amp"] | |
| default_agents = ["amp", "copilot", "claude", "codex", "cursor", "windsurf", "cline", "aider", "kilocode", "firebase", "gemini-cli", "crush", "openhands", "junie", "augmentcode", "opencode", "goose", "jules"] |
Changes
Technical Details
Testing
Generated with opencode by claude-3.5-sonnet
Summary by cubic
Skill repositories are now configurable via SKILLS.txt, so you can add or update skills without touching the Makefile. Dotdir syncing uses rsync, and Ruler defaults were simplified with .gitignore updates enabled.
Refactors
Migration
Written for commit 1a57d1a. Summary will update on new commits.