Skip to content

Skills sync revamp #124

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
shunkakinoki merged 2 commits into from
Apr 10, 2026
Merged

Skills sync revamp #124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2322d6c
feat: skills-sync-revamp
shunkakinoki Apr 10, 2026
aee2627
fix: skills-install cache validation and manifest recording
shunkakinoki Apr 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
165 changes: 149 additions & 16 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ RULES_TARGET_DIR := $(dir $(lastword $(MAKEFILE_LIST))).ruler
SKILLS_SRC_DIR := $(dir $(lastword $(MAKEFILE_LIST)))skills
SKILLS_RULER_DIR := $(dir $(lastword $(MAKEFILE_LIST))).ruler/skills
SKILLS_TARGET_DIRS := $(HOME)/.claude/skills $(HOME)/.cursor/skills $(HOME)/.codex/skills $(HOME)/.roo/skills $(HOME)/.gemini/skills $(HOME)/.agents/skills $(HOME)/.vibe/skills $(HOME)/.config/opencode/skills
SKILLS_STATE_DIR := $(HOME)/.cache/dotagents/skills
SKILLS_MANIFEST_DIR := $(SKILLS_STATE_DIR)/manifests
SKILLS_SPEC_STATE_FILE := $(SKILLS_STATE_DIR)/skills.txt.normalized
SKILLS_EXTERNAL_SOURCE_DIR := $(HOME)/.agents/skills

MCP_SRC := $(dir $(lastword $(MAKEFILE_LIST))).ruler/mcp.json
MCP_TARGET_DIRS := $(HOME)/.cursor $(HOME)/.claude $(HOME)/.codex
Expand All @@ -32,13 +36,12 @@ SKILLS_FILE := $(dir $(lastword $(MAKEFILE_LIST)))SKILLS.txt
ifeq ($(DOTAGENTS_SKIP_SYNC),)
.PHONY: sync
sync: ruler-prepare ## Sync project commands, skills, and MCP configuration to assistant-specific directories.
@make ruler-apply-global
@make commands-sync
@make skills-clean
@make skills-install
@make skills-sync
@make mcp-sync
@make ruler-dotdirs-sync
@$(MAKE) ruler-apply-global
@$(MAKE) commands-sync
@$(MAKE) skills-install
@$(MAKE) skills-sync
@$(MAKE) mcp-sync
@$(MAKE) ruler-dotdirs-sync
endif

.PHONY: ruler-prepare
Expand Down Expand Up @@ -81,23 +84,92 @@ ruler-rules-copy: ## Copy rules to .ruler directory.
# ====================================================================================

.PHONY: skills-clean
skills-clean: ## Remove all globally installed skills for a clean reinstall.
skills-clean: ## Remove all globally installed skills and cached install state.
@for target in $(SKILLS_TARGET_DIRS); do \
if [ -d "$$target" ]; then \
rm -rf "$$target"/*; \

Copilot AI Apr 10, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updated skills-clean description says it removes “all” skills, but the implementation uses rm -rf "$target"/*, which won’t remove dotfiles/directories (e.g., .foo). Either adjust the wording or update removal to include hidden entries so behavior matches the description.

Suggested change
rm -rf "$$target"/*; \
find "$$target" -mindepth 1 -maxdepth 1 -exec rm -rf -- {} +; \

Copilot uses AI. Check for mistakes.
echo "Cleaned $$target"; \
fi; \
done
@if [ -d "$(SKILLS_STATE_DIR)" ]; then \
rm -rf "$(SKILLS_STATE_DIR)"; \
echo "Cleared $(SKILLS_STATE_DIR)"; \
fi

.PHONY: skills-managed-clean
skills-managed-clean: ## Remove managed external skills recorded from SKILLS.txt.
@manifest_dir="$(SKILLS_MANIFEST_DIR)"; \
if [ -d "$$manifest_dir" ]; then \
for manifest in "$$manifest_dir"/*.skills; do \
if [ ! -f "$$manifest" ]; then \
continue; \
fi; \
while IFS= read -r skill || [ -n "$$skill" ]; do \
if [ -z "$$skill" ]; then \
continue; \
fi; \
for target in $(SKILLS_TARGET_DIRS); do \
if [ -e "$$target/$$skill" ] || [ -L "$$target/$$skill" ]; then \
rm -rf "$$target/$$skill"; \
echo "Removed $$target/$$skill"; \
fi; \
done; \
done < "$$manifest"; \
done; \
fi
@if [ -d "$(SKILLS_STATE_DIR)" ]; then \
rm -rf "$(SKILLS_STATE_DIR)"; \
echo "Cleared $(SKILLS_STATE_DIR)"; \
fi

.PHONY: skills-install
skills-install: ## Install skills from SKILLS.txt (supports per-repo skill selection).
@failed=0; \
grep -v '^\s*#' $(SKILLS_FILE) | grep -v '^\s*$$' | while IFS= read -r line; do \
repo=$$(echo "$$line" | awk '{print $$1}'); \
skill_args=$$(echo "$$line" | awk '{print $$2}' | tr ',' '\n' | sed '/^$$/d' | while read -r s; do printf " --skill $$s"; done); \
if [ -n "$$skill_args" ]; then \
skills-install: ## Ensure skills from SKILLS.txt are installed and reconcile managed removals.
@state_dir="$(SKILLS_STATE_DIR)"; \
manifest_dir="$(SKILLS_MANIFEST_DIR)"; \
spec_state="$(SKILLS_SPEC_STATE_FILE)"; \
external_source="$(SKILLS_EXTERNAL_SOURCE_DIR)"; \
tmp_spec=$$(mktemp); \

Copilot AI Apr 10, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mktemp is invoked without a template (e.g., mktemp). On macOS/BSD mktemp typically requires a template or -t, so this target can fail. Use a portable mktemp invocation (or allow TMPDIR) for the spec temp file.

Suggested change
tmp_spec=$$(mktemp); \
tmp_spec=$$(mktemp "$${TMPDIR:-/tmp}/skills-spec.XXXXXX"); \

Copilot uses AI. Check for mistakes.
spec_changed=0; \
failed=0; \
mkdir -p "$$state_dir" "$$manifest_dir" "$$external_source"; \
list_external_skills() { \
if [ -d "$$external_source" ]; then \
find "$$external_source" -mindepth 1 -maxdepth 1 \( -type d -o -type l \) -exec basename {} \; | LC_ALL=C sort -u; \
fi; \
}; \
remove_repo_skills() { \
manifest_file="$$1"; \
if [ ! -f "$$manifest_file" ]; then \
return 0; \
fi; \
while IFS= read -r skill || [ -n "$$skill" ]; do \
if [ -z "$$skill" ]; then \
continue; \
fi; \
for target in $(SKILLS_TARGET_DIRS); do \
if [ -e "$$target/$$skill" ] || [ -L "$$target/$$skill" ]; then \
rm -rf "$$target/$$skill"; \
fi; \
done; \
done < "$$manifest_file"; \
}; \
install_repo() { \
repo="$$1"; \
normalized_skills="$$2"; \
manifest_file="$$3"; \
cleanup_old="$$4"; \
before_file=$$(mktemp); \
after_file=$$(mktemp); \
Comment on lines +161 to +162

Copilot AI Apr 10, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mktemp is invoked without a template for before_file/after_file. This is not portable on macOS/BSD (often errors with “too few X's in template”). Use a portable mktemp pattern for these temp files as well.

Suggested change
before_file=$$(mktemp); \
after_file=$$(mktemp); \
before_file=$$(mktemp "$${TMPDIR:-/tmp}/before_file.XXXXXX"); \
after_file=$$(mktemp "$${TMPDIR:-/tmp}/after_file.XXXXXX"); \

Copilot uses AI. Check for mistakes.
if [ "$$cleanup_old" = "1" ]; then \
remove_repo_skills "$$manifest_file"; \
fi; \
list_external_skills > "$$before_file"; \
if [ -n "$$normalized_skills" ]; then \
skill_args=$$(printf '%s\n' "$$normalized_skills" | tr ',' '\n' | sed '/^$$/d' | while IFS= read -r s; do printf " --skill %s" "$$s"; done); \
echo "Installing selected skills from $$repo..."; \
if bunx skills add $$repo --global --yes $$skill_args </dev/null; then \
list_external_skills > "$$after_file"; \
comm -13 "$$before_file" "$$after_file" > "$$manifest_file"; \
echo "✓ Installed $$repo (selective)"; \
else \
echo "✗ Failed to install $$repo (continuing...)"; \
Expand All @@ -106,19 +178,80 @@ skills-install: ## Install skills from SKILLS.txt (supports per-repo skill selec
else \
echo "Installing all skills from $$repo..."; \
if bunx skills add $$repo --global --yes </dev/null; then \
list_external_skills > "$$after_file"; \
comm -13 "$$before_file" "$$after_file" > "$$manifest_file"; \
echo "✓ Installed $$repo (all)"; \
else \
echo "✗ Failed to install $$repo (continuing...)"; \
failed=1; \
fi; \
fi; \
done; \
rm -f "$$before_file" "$$after_file"; \
}; \
while IFS= read -r raw_line || [ -n "$$raw_line" ]; do \
line=$$(printf '%s' "$$raw_line" | sed 's/^[[:space:]]*//; s/[[:space:]]*$$//'); \
case "$$line" in \
''|\#*) continue ;; \
esac; \
repo=$$(printf '%s\n' "$$line" | awk '{print $$1}'); \
skills_csv=$$(printf '%s\n' "$$line" | awk '{print $$2}'); \
normalized_skills=$$(printf '%s\n' "$$skills_csv" | tr ',' '\n' | sed '/^$$/d' | LC_ALL=C sort | paste -sd, -); \
printf '%s|%s\n' "$$repo" "$$normalized_skills" >> "$$tmp_spec"; \
done < "$(SKILLS_FILE)"; \
if [ "$${DOTAGENTS_FORCE_SKILLS_INSTALL:-0}" = "1" ]; then \
spec_changed=1; \
echo "Forcing managed external skill reinstall..."; \
elif [ ! -f "$$spec_state" ] || ! cmp -s "$$tmp_spec" "$$spec_state"; then \
spec_changed=1; \
echo "Detected SKILLS.txt changes; refreshing managed external skills..."; \
fi; \
if [ "$$spec_changed" = "1" ]; then \
$(MAKE) skills-managed-clean; \
mkdir -p "$$state_dir" "$$manifest_dir" "$$external_source"; \
while IFS='|' read -r repo normalized_skills || [ -n "$$repo$$normalized_skills" ]; do \
manifest_file="$$manifest_dir/$$(printf '%s' "$$repo" | sed 's#[^A-Za-z0-9_.-]#_#g').skills"; \

@coderabbitai coderabbitai Bot Apr 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check for potential manifest filename collisions in SKILLS.txt
if [ -f "SKILLS.txt" ]; then
  echo "Repos in SKILLS.txt:"
  grep -v '^#' SKILLS.txt | grep -v '^$' | awk '{print $1}' | while read repo; do
    sanitized=$(echo "$repo" | sed 's#[^A-Za-z0-9_.-]#_#g')
    echo "  $repo -> $sanitized.skills"
  done | sort | uniq -d -f1
else
  echo "SKILLS.txt not found"
fi

Repository: shunkakinoki/dotagents

Length of output: 86

🏁 Script executed:

# Check if SKILLS.txt exists and examine its content
echo "=== Checking SKILLS.txt ==="
if [ -f "SKILLS.txt" ]; then
    echo "SKILLS.txt exists. First 30 lines (non-comment):"
    head -30 SKILLS.txt | grep -v '^#' | grep -v '^$'
    echo ""
    echo "Total non-empty, non-comment lines:"
    grep -v '^#' SKILLS.txt | grep -v '^$' | wc -l
else
    echo "SKILLS.txt not found"
fi

Repository: shunkakinoki/dotagents

Length of output: 1075

🏁 Script executed:

# Examine the Makefile context around line 212
echo "=== Makefile context (lines 200-220) ==="
sed -n '200,220p' Makefile

Repository: shunkakinoki/dotagents

Length of output: 1165

🏁 Script executed:

# Check for any actual collision cases by analyzing all repos
echo "=== Analyzing repos for collisions ==="
if [ -f "SKILLS.txt" ]; then
    grep -v '^#' SKILLS.txt | grep -v '^$' | awk '{print $1}' | while read repo; do
        sanitized=$(echo "$repo" | sed 's#[^A-Za-z0-9_.-]#_#g')
        echo "$sanitized"
    done | sort > /tmp/sanitized_names.txt
    
    # Find duplicates
    echo "Duplicate sanitized names:"
    uniq -d /tmp/sanitized_names.txt
    
    # Show what repos map to each duplicate
    if [ -s /tmp/sanitized_names.txt ]; then
        echo ""
        echo "Full mapping:"
        grep -v '^#' SKILLS.txt | grep -v '^$' | awk '{print $1}' | while read repo; do
            sanitized=$(echo "$repo" | sed 's#[^A-Za-z0-9_.-]#_#g')
            echo "$sanitized <- $repo"
        done | sort
    fi
fi

Repository: shunkakinoki/dotagents

Length of output: 1951

Potential manifest filename collision for different repos is theoretically possible but not present in current SKILLS.txt.

The sanitization sed 's#[^A-Za-z0-9_.-]#_#g' replaces / with _, so repos like owner/foo-bar and owner_foo-bar would produce the same manifest filename. Analysis of the 39 repos currently in SKILLS.txt confirms no collisions exist today; however, this remains a latent collision risk as the file grows.

Consider updating to a more collision-resistant scheme (e.g., double-underscore for slash: s#/#__#g) as a defensive measure.

🔧 Suggested fix 
-			manifest_file="$$manifest_dir/$$(printf '%s' "$$repo" | sed 's#[^A-Za-z0-9_.-]#_#g').skills"; \
+			manifest_file="$$manifest_dir/$$(printf '%s' "$$repo" | sed 's#/#__#g; s#[^A-Za-z0-9_.-]#_#g').skills"; \
📝 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.

Suggested change
manifest_file="$$manifest_dir/$$(printf '%s' "$$repo" | sed 's#[^A-Za-z0-9_.-]#_#g').skills"; \
manifest_file="$$manifest_dir/$$(printf '%s' "$$repo" | sed 's#/#__#g; s#[^A-Za-z0-9_.-]#_#g').skills"; \
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@Makefile` at line 212, The manifest filename generation using manifest_file
and sed 's#[^A-Za-z0-9_.-]#_#g' can produce collisions (e.g., owner/foo ->
owner_foo); update the sanitization used when computing manifest_file to make
slashes distinct or otherwise collision-resistant — for example, first map '/'
to a unique token like '__' (apply s#/#__#g before the generic sanitizer) or
append a short hash of $$repo (e.g., use a checksum of $$repo) to the sanitized
name; change the expression that builds manifest_file so it either replaces '/'
with '__' prior to the broad character replacement or includes the hash to
ensure uniqueness.

install_repo "$$repo" "$$normalized_skills" "$$manifest_file" 0; \
done < "$$tmp_spec"; \
Comment on lines +211 to +214

Copilot AI Apr 10, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manifest filenames are derived from a sanitized repo string (sed 's#[^A-Za-z0-9_.-]#_#g'). Different repos can collide to the same manifest name (e.g., a/b vs a_b), causing managed-clean/install state to mix. Consider using a collision-resistant encoding (e.g., include a hash of the repo) for manifest filenames.

Copilot uses AI. Check for mistakes.
else \
while IFS='|' read -r repo normalized_skills || [ -n "$$repo$$normalized_skills" ]; do \
manifest_file="$$manifest_dir/$$(printf '%s' "$$repo" | sed 's#[^A-Za-z0-9_.-]#_#g').skills"; \
reinstall_repo=0; \
if [ ! -f "$$manifest_file" ]; then \
reinstall_repo=1; \
echo "Reinstalling $$repo (missing manifest)"; \
else \
while IFS= read -r skill || [ -n "$$skill" ]; do \
if [ -z "$$skill" ]; then \
continue; \
fi; \
if [ ! -e "$$external_source/$$skill" ] && [ ! -L "$$external_source/$$skill" ]; then \
reinstall_repo=1; \
echo "Reinstalling $$repo (missing $$external_source/$$skill)"; \
break; \
fi; \
done < "$$manifest_file"; \
fi; \
if [ "$$reinstall_repo" = "1" ]; then \
install_repo "$$repo" "$$normalized_skills" "$$manifest_file" 1; \
else \
echo "Skipping $$repo (installed state matches SKILLS.txt)"; \
fi; \
done < "$$tmp_spec"; \
fi; \
cp "$$tmp_spec" "$$spec_state"; \
rm -f "$$tmp_spec"; \
if [ "$$failed" = "1" ]; then \
echo "Some skills failed to install (see above)."; \
else \
echo "All external skills installed successfully."; \
echo "Managed external skills are in sync."; \
fi
Comment on lines +241 to 247

Copilot AI Apr 10, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

skills-install performs critical filesystem state updates (cp to the spec state file) but the recipe doesn’t use set -e or check return codes, so a failure can still print “Managed external skills are in sync.” and leave the cache inconsistent. Consider failing the target on unexpected errors (while still allowing bunx skills add to be non-fatal if that’s intended).

Copilot uses AI. Check for mistakes.
Comment on lines 243 to 247

@coderabbitai coderabbitai Bot Apr 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

The target exits successfully even when installations fail.

When failed=1, the message is printed but the target exits with status 0. This masks failures in CI/CD pipelines and makes make sync report success even when some skills failed to install.

🐛 Proposed fix to propagate failure status 
 	if [ "$$failed" = "1" ]; then \
 		echo "Some skills failed to install (see above)."; \
+		exit 1; \
 	else \
 		echo "Managed external skills are in sync."; \
 	fi
📝 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.

Suggested change
if [ "$$failed" = "1" ]; then \
echo "Some skills failed to install (see above)."; \
else \
echo "All external skills installed successfully."; \
echo "Managed external skills are in sync."; \
fi
if [ "$$failed" = "1" ]; then \
echo "Some skills failed to install (see above)."; \
exit 1; \
else \
echo "Managed external skills are in sync."; \
fi
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@Makefile` around lines 243 - 247, The Makefile currently prints "Some skills
failed to install" when the variable failed is set but still exits with status
0, masking CI failures; modify the recipe that checks the failed variable (the
if [ "$$failed" = "1" ]; then ... fi block) to explicitly exit with a non‑zero
status in the failure branch (e.g., add an "exit 1" or "false" immediately after
the failure echo) so the make target (e.g., sync/managed-skills target that sets
failed) propagates failure to the caller; ensure the change remains inside the
same shell invocation (preserve the existing backslash line-continuations or use
.ONESHELL).


.PHONY: skills-refresh
skills-refresh: ## Force a clean reinstall of external skills and re-sync local repo skills.
@$(MAKE) skills-managed-clean

Copilot AI Apr 10, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

skills-refresh runs skills-managed-clean, then forces skills-install with DOTAGENTS_FORCE_SKILLS_INSTALL=1. But skills-install will call skills-managed-clean again when the force flag is set, doing the cleanup twice. Consider removing the first clean or adjusting skills-install to skip the clean when invoked from refresh.

Suggested change
@$(MAKE) skills-managed-clean

Copilot uses AI. Check for mistakes.
@DOTAGENTS_FORCE_SKILLS_INSTALL=1 $(MAKE) skills-install
@$(MAKE) skills-sync

.PHONY: skills-install-repo
skills-install-repo: ## Install a single skill repo. Usage: make skills-install-repo REPO=owner/repo [SKILLS=a,b,c]
@if [ -z "$(REPO)" ]; then \
Expand Down
8 changes: 7 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,10 @@ Rules for Agents
make sync
```

This runs the full pipeline: prepares `.ruler/`, generates agent instruction files (`~/.claude/CLAUDE.md`, etc.) via Ruler, then syncs commands, skills, MCP config, and dot directories to `$HOME`.
This runs the full pipeline: prepares `.ruler/`, generates agent instruction files (`~/.claude/CLAUDE.md`, etc.) via Ruler, reconciles managed external skills from `SKILLS.txt` against the actual installed skill directories, then syncs commands, local repo skills, MCP config, and dot directories to `$HOME`.

To force a clean reinstall of external skills, run:

```bash
make skills-refresh
```