chore(skills): centralize AI assistant config via symlinks#9951
chore(skills): centralize AI assistant config via symlinks#9951grauJavier wants to merge 3 commits intoprowler-cloud:masterfrom
Conversation
|
✅ Conflict Markers Resolved All conflict markers have been successfully resolved in this pull request. |
|
Hello @grauJavier! Thanks for this, we'll review it as soon as possible! 🙌 |
There was a problem hiding this comment.
The idea of centralizing with symlinks is great 👏
I tested the script locally and it works, the symlinks are created correctly. However, I found some issues that need to be fixed before merging:
🔴 Bugs
1. setup_copilot() - Symlink with absolute path
ln -sf "$REPO_ROOT/AGENTS.md" "$target"This creates a symlink with an absolute path (/Users/javier/prowler/AGENTS.md). If another dev clones the repo in a different location, the symlink points to a path that doesn't exist.
Fix:
ln -sf "../AGENTS.md" "$target"2. link_agents_md() - cd without protection
cd "$agents_dir" && ln -sf "$(basename "$agents_file")" "$target_name" && cd "$SCRIPT_DIR"If ln -sf fails, the script never returns to $SCRIPT_DIR and subsequent iterations run from the wrong directory.
Fix: Use a subshell:
(cd "$agents_dir" && ln -sf "$(basename "$agents_file")" "$target_name")3. Missing .codex/ in gitignore
setup_claude() and setup_gemini() add to gitignore, but setup_codex() doesn't:
setup_codex() {
# ...
# Missing: add_to_gitignore ".codex/"
}🟡 Minor
4. Tests don't validate symlinks
Tests use assert_file_exists which passes for both files and symlinks. They should be updated to use assert_symlink_exists to explicitly validate the new behavior. Also rename the section from "Copy tests" to "Link tests".
5. Header comments outdated
Line 4 says CLAUDE.md copies but now they're symlinks.
Once these are fixed, the PR is ready to merge!
|
Thanks for the detailed review! I've addressed all the points: 🔴 Fixed
🟡 Improvements
Verified locally, all 19 tests passed! ✅ |
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
Sorry for the late reply on this one, Javier.
All three bugs from the first review are properly fixed — the relative path for copilot, the subshell wrapper, and the codex gitignore. Tests updated correctly too. Solid work.
Three minor things I noticed, curious what you think:
-
Header line 7 still says "copy" —
# - GitHub Copilot: .github/copilot-instructions.md copybutsetup_copilot()now usesln -sf. Should probably say "symlink" for consistency with lines 4-5. -
Unused variable in
link_agents_md()—local target_path="$agents_dir/$target_name"is declared but never referenced. Looks like leftover from the refactor. -
Summary message on line 305 —
"AGENTS.md is the source of truth - edit it, then re-run this script."With symlinks, the whole point is you don't need to re-run. Something like"AGENTS.md is the source of truth - changes are reflected automatically via symlinks."would be more accurate.
None of these are blockers — let me know if you agree or if there's a reason I'm missing.
- Update copilot header comment from 'copy' to 'symlink' - Remove unused target_path variable in link_agents_md() - Update summary message to reflect symlinks auto-propagate changes - Use assert_symlink_exists in idempotent test for CLAUDE.md
Context
Currently, developers using different AI assistants (Claude, Gemini, etc.) end up with duplicated skill files and assistant configurations. This creates maintenance friction, as the root
AGENTS.mdandskills/directory should be the single source of truth, but generated files can easily drift out of sync.Description
Updated
skills/setup.shto use ALWAYS symbolic links instead of copying files..claude/skills,.gemini/skills, etc., as symlinks pointing to the rootskills/directory.CLAUDE.md) as symlinks pointing toAGENTS.md..gitignoreto maintain a clean environment when different developers use different assistants.Steps to review
./skills/setup.shlocally..claude/skillsis a symlink pointing to.../skills..claude/CLAUDE.mdis a symlink pointing to.../AGENTS.md.