feat: add selective skill installation to reduce context token usage - #107
Conversation
- SKILLS.txt now supports per-repo skill selection (repo skill1,skill2,...) - skills-install parses new format and passes --skill flag to bunx - Add skills-clean target, integrate into make sync - Curate essential skills per repo (828 -> ~193 skills, ~76% reduction) - Estimated context savings: ~62k -> ~15k tokens
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe Makefile's skill-install workflow now cleans existing global skills, then parses Changes
Sequence DiagramsequenceDiagram
participant User
participant Makefile as Makefile (sync)
participant CommandsSync as commands-sync
participant SkillsClean as skills-clean
participant SKILLSFile as SKILLS.txt
participant SkillsInstall as skills-install
participant SkillsInstallRepo as skills-install-repo
User->>Makefile: make sync
Makefile->>CommandsSync: run commands-sync
CommandsSync-->>Makefile: done
Makefile->>SkillsClean: run skills-clean
SkillsClean->>SkillsClean: remove contents of $(SKILLS_TARGET_DIRS)
SkillsClean-->>Makefile: done
Makefile->>SkillsInstall: run skills-install
SkillsInstall->>SKILLSFile: read lines (skip comments/blank)
SKILLSFile-->>SkillsInstall: repo + optional skill list
loop per repo entry
SkillsInstall->>SkillsInstallRepo: invoke with REPO (+ SKILLS if present)
alt SKILLS specified
SkillsInstallRepo->>SkillsInstallRepo: bunx skills add ... --skill each
else no SKILLS
SkillsInstallRepo->>SkillsInstallRepo: bunx skills add (all)
end
SkillsInstallRepo-->>SkillsInstall: repo install complete
end
SkillsInstall-->>Makefile: done
Makefile-->>User: sync finished
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Makefile (1)
34-37: Prefer$(MAKE)for recursive targets.Using
$(MAKE)preserves flags/jobserver behavior across nested invocations.Proposed fix
- `@make` ruler-apply-global + @$(MAKE) ruler-apply-global `@make` commands-sync - `@make` skills-clean - `@make` skills-install - `@make` skills-sync - `@make` mcp-sync - `@make` ruler-dotdirs-sync + @$(MAKE) skills-clean + @$(MAKE) skills-install + @$(MAKE) skills-sync + @$(MAKE) mcp-sync + @$(MAKE) ruler-dotdirs-sync🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 34 - 37, Replace literal make invocations with $(MAKE) for recursive targets to preserve flags and jobserver behavior: change the invocations of ruler-apply-global, commands-sync, skills-clean, and skills-install (currently called via "make") to use "$(MAKE) ruler-apply-global", "$(MAKE) commands-sync", "$(MAKE) skills-clean", and "$(MAKE) skills-install" respectively so recursive make inherits flags and -j behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 83-86: The cleanup loop over SKILLS_TARGET_DIRS currently only
removes non-hidden entries via rm -rf "$$target"/*; update the loop that
iterates over $(SKILLS_TARGET_DIRS) (the shell variable target) to remove all
entries including dotfiles safely—for example replace the rm line with a safe
command that removes every child entry without touching the parent directory
such as using find: find "$$target" -mindepth 1 -maxdepth 1 -exec rm -rf -- {} +
so hidden files and dirs are also removed while avoiding removing "." or "..".
In `@SKILLS.txt`:
- Line 34: Replace the lowercase platform label "github/awesome-copilot" with
the official-casing "GitHub/awesome-copilot" in SKILLS.txt (update the string
"github/awesome-copilot (257 total) - keep essential dev/infra skills" to use
"GitHub") so the comment text uses the official platform casing.
---
Nitpick comments:
In `@Makefile`:
- Around line 34-37: Replace literal make invocations with $(MAKE) for recursive
targets to preserve flags and jobserver behavior: change the invocations of
ruler-apply-global, commands-sync, skills-clean, and skills-install (currently
called via "make") to use "$(MAKE) ruler-apply-global", "$(MAKE) commands-sync",
"$(MAKE) skills-clean", and "$(MAKE) skills-install" respectively so recursive
make inherits flags and -j 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0306bcb2-8de6-4ae9-9ba2-803003ed6d60
📒 Files selected for processing (2)
MakefileSKILLS.txt
| @for target in $(SKILLS_TARGET_DIRS); do \ | ||
| if [ -d "$$target" ]; then \ | ||
| rm -rf "$$target"/*; \ | ||
| echo "Cleaned $$target"; \ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the Makefile
fd -t f "Makefile$" --maxdepth 2Repository: shunkakinoki/dotagents
Length of output: 74
🏁 Script executed:
# Look for the Makefile and examine the context around lines 83-86
if [ -f "Makefile" ]; then
echo "=== Makefile found ==="
sed -n '75,95p' Makefile
fiRepository: shunkakinoki/dotagents
Length of output: 895
🏁 Script executed:
# Test rm -rf behavior with hidden files to verify the claim
# Create a test directory structure to validate the issue
mkdir -p /tmp/test_cleanup/testdir
cd /tmp/test_cleanup/testdir
# Add visible and hidden files
touch visible_file.txt .hidden_file .another_hidden
mkdir visible_dir .hidden_dir
# Test 1: rm -rf testdir/*
echo "Before rm -rf testdir/*:"
ls -la testdir/
# Clone the directory for testing
cp -r testdir testdir_copy1
rm -rf testdir_copy1/*
echo "After rm -rf testdir_copy1/*:"
ls -la testdir_copy1/
echo "Hidden files remain: $(ls -la testdir_copy1/ | grep -c '^\.')"
# Test 2: The proposed fix pattern
cp -r testdir testdir_copy2
rm -rf testdir_copy2/* testdir_copy2/.[!.]* testdir_copy2/..?*
echo "After proposed fix (rm -rf testdir_copy2/* testdir_copy2/.[!.]* testdir_copy2/..?*):"
ls -la testdir_copy2/
echo "Remaining items: $(ls -la testdir_copy2/ | wc -l)"Repository: shunkakinoki/dotagents
Length of output: 674
🏁 Script executed:
# Test rm -rf behavior with hidden files
mkdir -p /tmp/rm_test/target
cd /tmp/rm_test
# Create test files and directories
touch target/visible_file.txt
touch target/.hidden_file
touch target/.bashrc
mkdir target/.hidden_dir
mkdir target/visible_dir
echo "=== Before rm -rf target/* ==="
ls -la target/ | tail -n +4
# Test the current approach
rm -rf target/*
echo ""
echo "=== After rm -rf target/* ==="
ls -la target/ | tail -n +4
if [ -f target/.hidden_file ] || [ -d target/.hidden_dir ]; then
echo "CONFIRMED: Hidden files/dirs remain after rm -rf target/*"
else
echo "Hidden files were removed"
fiRepository: shunkakinoki/dotagents
Length of output: 664
🏁 Script executed:
# Now test the proposed fix
mkdir -p /tmp/rm_test2/target
cd /tmp/rm_test2
# Create test files
touch target/visible_file.txt
touch target/.hidden_file
touch target/.bashrc
mkdir target/.hidden_dir
echo "=== Before proposed fix ==="
ls -la target/ | tail -n +4
# Test the proposed fix: rm -rf target/* target/.[!.]* target/..?*
rm -rf target/* target/.[!.]* target/..?*
echo ""
echo "=== After proposed fix (rm -rf target/* target/.[!.]* target/..?*) ==="
ls -la target/ | tail -n +4
remaining=$(ls -la target/ | wc -l)
if [ "$remaining" -le 3 ]; then
echo "SUCCESS: All files removed (only . and .. remain)"
else
echo "Some files remain"
fiRepository: shunkakinoki/dotagents
Length of output: 457
skills-clean does not remove hidden files, leaving partial cleanup that can affect reinstall behavior.
Line 85 only deletes *, which excludes dotfiles and dotdirectories. These remnants can interfere with a clean reinstall.
Proposed fix
- rm -rf "$$target"/*; \
+ rm -rf "$$target"/* "$$target"/.[!.]* "$$target"/..?*; \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @for target in $(SKILLS_TARGET_DIRS); do \ | |
| if [ -d "$$target" ]; then \ | |
| rm -rf "$$target"/*; \ | |
| echo "Cleaned $$target"; \ | |
| `@for` target in $(SKILLS_TARGET_DIRS); do \ | |
| if [ -d "$$target" ]; then \ | |
| rm -rf "$$target"/* "$$target"/.[!.]* "$$target"/..?*; \ | |
| echo "Cleaned $$target"; \ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 83 - 86, The cleanup loop over SKILLS_TARGET_DIRS
currently only removes non-hidden entries via rm -rf "$$target"/*; update the
loop that iterates over $(SKILLS_TARGET_DIRS) (the shell variable target) to
remove all entries including dotfiles safely—for example replace the rm line
with a safe command that removes every child entry without touching the parent
directory such as using find: find "$$target" -mindepth 1 -maxdepth 1 -exec rm
-rf -- {} + so hidden files and dirs are also removed while avoiding removing
"." or "..".
| # getsentry/skills (24 total) - keep dev workflow skills | ||
| getsentry/skills agents-md,claude-settings-audit,code-review,code-simplifier,commit,create-branch,create-pr,find-bugs,gh-review-requests,gha-security-review,iterate-pr,pr-writer,security-review | ||
|
|
||
| # github/awesome-copilot (257 total) - keep essential dev/infra skills |
There was a problem hiding this comment.
Use official platform casing in comment text.
Please change github/awesome-copilot description label text to GitHub for consistency/readability.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~34-~34: The official name of this software platform is spelled with a capital “H”.
Context: ...iterate-pr,pr-writer,security-review # github/awesome-copilot (257 total) - keep esse...
(GITHUB)
[uncategorized] ~34-~34: The official name of this software platform is spelled with a capital “H”.
Context: ...otal) - keep essential dev/infra skills github/awesome-copilot chrome-devtools,convent...
(GITHUB)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SKILLS.txt` at line 34, Replace the lowercase platform label
"github/awesome-copilot" with the official-casing "GitHub/awesome-copilot" in
SKILLS.txt (update the string "github/awesome-copilot (257 total) - keep
essential dev/infra skills" to use "GitHub") so the comment text uses the
official platform casing.
There was a problem hiding this comment.
Pull request overview
Adds selective, per-repo skill installation support to reduce the globally installed skill set (and therefore context token usage) during make sync.
Changes:
- Replaces
SKILLS.txtwith a new format that optionally lists a comma-separated subset of skills per repo. - Updates
skills-installto parse the new format and callbunx skills add ... --skill "a,b,c"when a subset is provided. - Adds
skills-cleanand wires it intomake syncto force clean reinstalls.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
SKILLS.txt |
Converts list of repos into a curated repo→skills mapping (or “install all”) to reduce installed skill volume. |
Makefile |
Adds skills-clean, updates skills-install parsing/installation logic, and adjusts sync flow to include cleaning and global ruler application. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if bunx skills add $$repo --global --yes; then \ | ||
| echo "✓ Installed $$repo"; \ | ||
| skills-install: ## Install skills from SKILLS.txt (supports per-repo skill selection). | ||
| @grep -v '^\s*#' $(SKILLS_FILE) | grep -v '^\s*$$' | while IFS= read -r line; do \ |
There was a problem hiding this comment.
The grep filters use \s (e.g., grep -v '^\s*#'), but standard grep (BRE) doesn’t recognize \s as whitespace. This will fail to strip comments/blank lines and can result in trying to install a repo named # or an empty repo. Use POSIX character classes like ^[[:space:]]*# / ^[[:space:]]*$, or replace the whole pipeline with a single awk that skips comment/empty lines.
| @grep -v '^\s*#' $(SKILLS_FILE) | grep -v '^\s*$$' | while IFS= read -r line; do \ | |
| @awk '!/^[[:space:]]*#/ && NF' $(SKILLS_FILE) | while IFS= read -r line; do \ |
| skills-install: ## Install skills from SKILLS.txt (supports per-repo skill selection). | ||
| @grep -v '^\s*#' $(SKILLS_FILE) | grep -v '^\s*$$' | while IFS= read -r line; do \ | ||
| repo=$$(echo "$$line" | awk '{print $$1}'); \ | ||
| skills=$$(echo "$$line" | awk '{print $$2}'); \ | ||
| if [ -n "$$skills" ]; then \ | ||
| echo "Installing selected skills from $$repo ($$skills)..."; \ | ||
| if bunx skills add $$repo --global --yes --skill "$$skills"; then \ | ||
| echo "✓ Installed $$repo (selective)"; \ | ||
| else \ | ||
| echo "✗ Failed to install $$repo"; \ | ||
| exit 1; \ | ||
| fi; \ | ||
| else \ | ||
| echo "✗ Failed to install $$repo"; \ | ||
| exit 1; \ | ||
| echo "Installing all skills from $$repo..."; \ | ||
| if bunx skills add $$repo --global --yes; then \ | ||
| echo "✓ Installed $$repo (all)"; \ | ||
| else \ | ||
| echo "✗ Failed to install $$repo"; \ | ||
| exit 1; \ | ||
| fi; \ | ||
| fi; \ | ||
| done | ||
| @echo "All external skills installed successfully." |
There was a problem hiding this comment.
skills-install will report success even if $(SKILLS_FILE) is missing/unreadable: in a pipeline, the exit code is taken from the final while ...; do ...; done, which will exit 0 if it reads no lines, masking grep errors. Add an explicit existence/readability check for $(SKILLS_FILE) (and ideally fail if it contains no valid entries) before starting the install loop.
| sync: ruler-prepare ## Sync project commands, skills, and MCP configuration to assistant-specific directories. | ||
| @make ruler-apply-global |
There was a problem hiding this comment.
sync already has ruler-prepare as a prerequisite, and the recipe also calls make ruler-apply-global which itself depends on ruler-prepare. Because these are .PHONY, this runs the prepare steps twice on every make sync. Consider making sync depend on ruler-apply-global (instead of ruler-prepare) and removing the explicit make ruler-apply-global line, or otherwise restructuring so ruler-prepare only runs once per sync.
| sync: ruler-prepare ## Sync project commands, skills, and MCP configuration to assistant-specific directories. | |
| @make ruler-apply-global | |
| sync: ruler-apply-global ## Sync project commands, skills, and MCP configuration to assistant-specific directories. |
| skills-clean: ## Remove all globally installed skills for a clean reinstall. | ||
| @for target in $(SKILLS_TARGET_DIRS); do \ | ||
| if [ -d "$$target" ]; then \ | ||
| rm -rf "$$target"/*; \ |
There was a problem hiding this comment.
skills-clean uses rm -rf "$$target"/*, which won’t remove dotfiles/directories (e.g., .git, .DS_Store) under the skills dir. If hidden entries can exist, the cleanup will be incomplete and may interfere with a clean reinstall. Consider using find "$$target" -mindepth 1 -maxdepth 1 -exec rm -rf {} + (or equivalent) to remove all entries.
| rm -rf "$$target"/*; \ | |
| find "$$target" -mindepth 1 -maxdepth 1 -exec rm -rf {} +; \ |
Summary
repo skill1,skill2,...)skills-installMakefile target to parse new format and pass--skillflagskills-cleantarget and integrate intomake syncfor clean reinstallsTest plan
make syncand verify only selected skills are installednpx skills add <repo> --global --yes --skill "a,b"works for selective installskills-cleanremoves all skill directories/contextshows reduced skill token usage🤖 Generated with Claude Code
Summary by cubic
Adds per-repo selective skill installation to cut context token usage. Curates
SKILLS.txtfrom 828 to ~193 skills across 34 repos, reducing context from ~62k to ~15k tokens (~76%).New Features
SKILLS.txtformat supports per-repo skill lists:repo skill1,skill2,...(omit to install all; comments/blank lines allowed).skills-installparses the file and passes--skilltobunx skills addfor selective installs.skills-cleanand run it inmake syncfor clean reinstalls;skills-install-reponow supportsSKILLS=a,b,c.Migration
SKILLS.txtusing the new format where needed.make syncto clean and reinstall selected skills.make skills-install-repo REPO=owner/repo SKILLS=a,b.Written for commit 7fc07fa. Summary will update on new commits.