Skills sync revamp - #124
Conversation
- Cache check: only validate ~/.agents/skills/ (source of truth), not all target dirs which get modified by IDEs - Manifest recording: use before/after diff for selective installs so phantom skills (requested but nonexistent) don't cause perpetual reinstalls
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe Makefile is enhanced with a state and manifest tracking system for external skills management. New directories and files track normalized SKILLS.txt specifications, installed skill manifests, and state information. The Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CI
participant Make as Makefile
participant Spec as SKILLS.txt
participant State as State Files
participant Cache as Skills Cache<br/>(SKILLS_EXTERNAL_SOURCE_DIR)
participant Repos as Installation<br/>Targets
User->>Make: make skills-install
Make->>Spec: Read and normalize SKILLS.txt
Make->>State: Generate normalized spec state
Make->>State: Compare with previous spec state
alt Spec Changed or Force Install
Make->>Make: Run skills-managed-clean
Make->>State: Clear SKILLS_STATE_DIR
Make->>Repos: Clean managed external skills from all targets
Make->>Cache: Install all skills from SKILLS.txt
Make->>State: Create/update manifests for installed skills
else Spec Unchanged
Make->>State: Check per-repo manifest existence
alt Manifest Missing or Skills Missing
Make->>Cache: Targeted skill installation for affected repos
Make->>State: Update manifest with newly installed skills
else All Manifests Valid
Make->>Make: Skip reinstall
end
end
Make->>State: Write updated spec state file
Make->>User: Report: "managed external skills are in sync"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
There was a problem hiding this comment.
Pull request overview
Revamps the make sync workflow’s external skill handling by introducing a cached “managed skills” reconciliation mechanism driven by SKILLS.txt, plus a convenience target to force reinstall.
Changes:
- Update
make syncto install/reconcile external skills without doing a full skills wipe each run. - Add managed-skill state/manifest tracking under
~/.cache/dotagents/skills, plusskills-managed-cleanandskills-refreshtargets. - Update README to document the new behavior and the
skills-refreshworkflow.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| README.md | Documents the new skills reconciliation behavior and how to force a reinstall. |
| Makefile | Implements managed external skills reconciliation with cached state/manifests and adds skills-refresh. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| manifest_dir="$(SKILLS_MANIFEST_DIR)"; \ | ||
| spec_state="$(SKILLS_SPEC_STATE_FILE)"; \ | ||
| external_source="$(SKILLS_EXTERNAL_SOURCE_DIR)"; \ | ||
| tmp_spec=$$(mktemp); \ |
There was a problem hiding this comment.
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.
| tmp_spec=$$(mktemp); \ | |
| tmp_spec=$$(mktemp "$${TMPDIR:-/tmp}/skills-spec.XXXXXX"); \ |
| before_file=$$(mktemp); \ | ||
| after_file=$$(mktemp); \ |
There was a problem hiding this comment.
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.
| before_file=$$(mktemp); \ | |
| after_file=$$(mktemp); \ | |
| before_file=$$(mktemp "$${TMPDIR:-/tmp}/before_file.XXXXXX"); \ | |
| after_file=$$(mktemp "$${TMPDIR:-/tmp}/after_file.XXXXXX"); \ |
| 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"/*; \ |
There was a problem hiding this comment.
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.
| rm -rf "$$target"/*; \ | |
| find "$$target" -mindepth 1 -maxdepth 1 -exec rm -rf -- {} +; \ |
| 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"; \ | ||
| install_repo "$$repo" "$$normalized_skills" "$$manifest_file" 0; \ | ||
| done < "$$tmp_spec"; \ |
There was a problem hiding this comment.
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.
|
|
||
| .PHONY: skills-refresh | ||
| skills-refresh: ## Force a clean reinstall of external skills and re-sync local repo skills. | ||
| @$(MAKE) skills-managed-clean |
There was a problem hiding this comment.
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.
| @$(MAKE) skills-managed-clean |
| 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 |
There was a problem hiding this comment.
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).
![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)
208-210: Consider handlingskills-managed-cleanfailure.If
$(MAKE) skills-managed-cleanfails (e.g., permission error), the script continues silently and may leave stale skills. Consider adding error handling or at least a warning.🔧 Optional: Add error handling for clean step
if [ "$$spec_changed" = "1" ]; then \ - $(MAKE) skills-managed-clean; \ + if ! $(MAKE) skills-managed-clean; then \ + echo "Warning: skills-managed-clean failed; continuing with reinstall..."; \ + fi; \ mkdir -p "$$state_dir" "$$manifest_dir" "$$external_source"; \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 208 - 210, The Makefile currently runs `$(MAKE) skills-managed-clean` when `$$spec_changed` is 1 but ignores failures; update the block so a non-zero exit from the `skills-managed-clean` target is detected and handled (e.g., emit a clear error via echo/process logger and exit non‑zero or at least print a warning), before proceeding to `mkdir -p "$$state_dir" "$$manifest_dir" "$$external_source"`, ensuring `skills-managed-clean` failures do not allow the script to continue silently and leave stale state.
🤖 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 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).
- 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.
---
Nitpick comments:
In `@Makefile`:
- Around line 208-210: The Makefile currently runs `$(MAKE)
skills-managed-clean` when `$$spec_changed` is 1 but ignores failures; update
the block so a non-zero exit from the `skills-managed-clean` target is detected
and handled (e.g., emit a clear error via echo/process logger and exit non‑zero
or at least print a warning), before proceeding to `mkdir -p "$$state_dir"
"$$manifest_dir" "$$external_source"`, ensuring `skills-managed-clean` failures
do not allow the script to continue silently and leave stale state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| $(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"; \ |
There was a problem hiding this comment.
🧩 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"
fiRepository: 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"
fiRepository: 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' MakefileRepository: 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
fiRepository: 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.
| 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.
| 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 |
There was a problem hiding this comment.
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.
| 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).
Summary by cubic
Revamps skills sync to be incremental and stateful. External skills from
SKILLS.txtare reconciled via manifests and cache, cutting unnecessary reinstalls and speeding upmake sync.New Features
~/.cache/dotagents/skillswith per-repo manifests inmanifests/*.skills.make skills-refreshfor a clean reinstall andskills-managed-cleanfor managed cleanup.skills-installnormalizesSKILLS.txt, records installs via before/after diffs, and skips when state matches.make syncuses$(MAKE)and no longer performs a blanketskills-cleanby default.make skills-refreshusage.Bug Fixes
~/.agents/skills(source of truth), avoiding noise from IDE-modified target dirs.Written for commit aee2627. Summary will update on new commits.