-
Notifications
You must be signed in to change notification settings - Fork 89
Add context-aware skills with additionalDirectories support #14
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.
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds project/global skills distinction in docs and updates examples. Enhances find-skills to aggregate from current project, additionalDirectories, and global skills, with path discovery, deduplication, shadowing, tagging, and revised output/search behavior. Bumps using-skills docs version and updates descriptions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant FS as find-skills
participant CP as Current Project (.claude/skills)
participant AD as Additional Dirs (.claude/settings*.json)
participant GS as Global Skills
U->>FS: run find-skills [pattern]
rect rgba(230,245,255,0.5)
note over FS: Discover contexts
FS->>CP: Locate project skills dir
FS->>AD: Walk up to Git root<br/>read .claude/settings*.json<br/>collect additionalDirectories
AD-->>FS: Paths (deduped, normalized)
FS->>GS: Resolve global skills root
end
rect rgba(240,255,240,0.5)
note over FS: Collect & shadow
FS->>CP: List skills (exclude .claude in search)
FS->>AD: For each dir: list skills<br/>tag with (project-name)
FS->>GS: List global skills
FS->>FS: Apply shadowing: CP > AD > GS
end
alt pattern provided
FS->>FS: Filter by pattern<br/>log search
else no pattern
FS->>FS: Use full sets
end
rect rgba(255,250,230,0.6)
note over FS: Emit results
FS-->>U: Project skills (no tag)
FS-->>U: Additional skills (with location tag)
FS-->>U: Global skills (no tag)
FS-->>U: "Use ..." lines when when_to_use present
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (7)
skills/meta/writing-skills/SKILL.md (3)
82-90: Add language to fenced code blocks (markdownlint MD040).Specify a language for the directory-layout code fences to satisfy linters.
Apply:
-``` +```text ${SUPERPOWERS_SKILLS_ROOT}/ skills/ category/ skill-name/ SKILL.md # Main reference (required) supporting-file.* # Only if needed```diff -``` +```text .claude/ skills/ category/ skill-name/ SKILL.md # Same structure as global tool/ # Executable scripts if neededBased on static analysis hints Also applies to: 93-101 --- `62-66`: **Minor wording tweak for clarity.** Add an article. ```diff -**Don't create skill for:** +**Don't create a skill for:**Based on static analysis hints
102-107: Tighten phrasing in “Key differences.”Slight grammar/clarity improvement.
-**Key differences:** -- Global skills: Broadly applicable, personal working branch -- Project skills: Team-shared, version-controlled -- Current dir project skills shadow global skills when paths match -- Additional context project skills (from additionalDirectories) shown with location tags +**Key differences:** +- Global skills: broadly applicable, personal working branch +- Project skills: team-shared, version-controlled +- Project skills in the current directory shadow global skills when paths match +- Additional-context project skills (from additionalDirectories) are shown with location tagsBased on static analysis hints
skills/using-skills/find-skills (4)
166-170: Ensure search log directory exists before writing.Avoid silently dropping logs when the directory is missing.
if [[ -n "$PATTERN" ]]; then timestamp=$(date -u +"%Y-%m-%dT%H:%M:%SZ") - echo "{\"timestamp\":\"$timestamp\",\"query\":\"$PATTERN\"}" >> "$LOG_FILE" 2>/dev/null || true + mkdir -p "$SUPERPOWERS_DIR" 2>/dev/null || true + echo "{\"timestamp\":\"$timestamp\",\"query\":\"$PATTERN\"}" >> "$LOG_FILE" 2>/dev/null || true fi
67-76: Support tilde-expanded paths in additionalDirectories.Users often put "~/something" in settings; expand it safely.
# Resolve relative paths from settings_dir, absolute paths as-is - if [[ "$dir" = /* ]]; then + # Expand leading ~ to $HOME + if [[ "$dir" == "~" ]] || [[ "$dir" == "~/"* ]]; then + dir="${HOME}${dir#~}" + fi + + if [[ "$dir" = /* ]]; then # Absolute path - use as is dir_normalized=$(cd "$dir" 2>/dev/null && pwd) || continue else
29-41: Prefer jq for parsing JSON; fall back to awk.Awk parsing is brittle for formatting/escaping; use jq when available.
- if [[ -f "$settings_file" ]]; then - # Extract paths from additionalDirectories array only - # Output format: "path|settings_dir" so we can resolve relative paths correctly - awk -v settings_dir="$check_dir" ' - /"additionalDirectories"/ { in_section=1; next } - in_section && /\]/ { in_section=0; next } - in_section && /^[[:space:]]*"/ { - gsub(/^[[:space:]]*"|"[[:space:]]*,?[[:space:]]*$/, "") - print $0 "|" settings_dir - } - ' "$settings_file" 2>/dev/null - fi + if [[ -f "$settings_file" ]]; then + # Prefer jq if present + if command -v jq >/dev/null 2>&1; then + jq -r '(.additionalDirectories // [])[]?' "$settings_file" 2>/dev/null \ + | while IFS= read -r p; do printf '%s|%s\n' "$p" "$check_dir"; done + else + # Fallback: naive awk extraction of string entries in additionalDirectories + awk -v settings_dir="$check_dir" ' + /"additionalDirectories"/ { in_section=1; next } + in_section && /\]/ { in_section=0; next } + in_section && /^[[:space:]]*"/ { + gsub(/^[[:space:]]*"|"[[:space:]]*,?[[:space:]]*$/, "") + print $0 "|" settings_dir + } + ' "$settings_file" 2>/dev/null + fi + fi
267-284: Optional: preserve context grouping in output.Sorting mixes project/additional/global. Consider grouped output (project first, then additional with tags, then global) for readability.
I can provide a grouped-print variant if desired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
skills/meta/writing-skills/SKILL.md(2 hunks)skills/using-skills/SKILL.md(1 hunks)skills/using-skills/find-skills(5 hunks)
🧰 Additional context used
🪛 LanguageTool
skills/meta/writing-skills/SKILL.md
[grammar] ~56-~56: There might be a mistake here.
Context: ... project skill (.claude/skills/) when:** - Team-specific workflows and conventions ...
(QB_NEW_EN)
[grammar] ~57-~57: There might be a mistake here.
Context: ... Team-specific workflows and conventions - Project architecture patterns - Domain-s...
(QB_NEW_EN)
[grammar] ~58-~58: There might be a mistake here.
Context: ...ventions - Project architecture patterns - Domain-specific best practices - Onboard...
(QB_NEW_EN)
[grammar] ~59-~59: There might be a mistake here.
Context: ...atterns - Domain-specific best practices - Onboarding patterns for this codebase *...
(QB_NEW_EN)
[grammar] ~62-~62: There might be a mistake here.
Context: ...this codebase Don't create skill for: - One-off solutions - Standard practices w...
(QB_NEW_EN)
[grammar] ~63-~63: There might be a mistake here.
Context: ... create skill for:** - One-off solutions - Standard practices well-documented elsew...
(QB_NEW_EN)
[grammar] ~64-~64: There might be a mistake here.
Context: ...dard practices well-documented elsewhere - Simple reminders (use CLAUDE.md instead)...
(QB_NEW_EN)
[grammar] ~102-~102: There might be a mistake here.
Context: ...cripts if needed ``` Key differences: - Global skills: Broadly applicable, perso...
(QB_NEW_EN)
[grammar] ~103-~103: There might be a mistake here.
Context: ...adly applicable, personal working branch - Project skills: Team-shared, version-con...
(QB_NEW_EN)
[grammar] ~104-~104: There might be a mistake here.
Context: ... skills: Team-shared, version-controlled - Current dir project skills shadow global...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.18.1)
skills/meta/writing-skills/SKILL.md
82-82: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
93-93: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (1)
skills/using-skills/SKILL.md (1)
3-5: LGTM: version bump + description align with context-aware project skills.Matches new additionalDirectories behavior and output semantics.
| if echo "$seen_skills_list" | grep -q "^${skill_path}$"; then | ||
| continue | ||
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use fixed-string, exact-line matching for shadowing checks.
Regex matching can misidentify paths containing regex metacharacters, breaking shadowing.
Apply:
- if echo "$seen_skills_list" | grep -q "^${skill_path}$"; then
+ if echo "$seen_skills_list" | grep -F -x -q "${skill_path}"; then
continue
fi- echo "$seen_skills_list" | grep -q "^${skill_path}$" && continue
+ echo "$seen_skills_list" | grep -F -x -q "${skill_path}" && continueAlso applies to: 238-239
🤖 Prompt for AI Agents
In skills/using-skills/find-skills around lines 212-214 (and also apply the same
change at 238-239): the current grep uses regex matching which misinterprets
skill paths containing regex metacharacters; change the grep invocation to use
fixed-string (-F) and exact-line (-x) matching (e.g., grep -Fx) when checking if
a skill_path is in seen_skills_list so it only matches the full literal line;
keep the surrounding logic (if ... then continue fi) unchanged.
- Parse additionalDirectories from .claude/settings*.json files - Walk up from current dir to git root looking for settings files - Resolve relative paths from where settings file is located - Search current dir .claude/skills/ (no location tag) - Search additional context dirs .claude/skills/ (with location tags) - Show project name as location tag for skills from additionalDirectories - Current dir skills shadow both additional context and global skills - Filter out current dir from additionalDirectories to avoid duplicates - Update documentation in writing-skills/SKILL.md for project skills This replaces the hierarchical walk-up approach with context-aware discovery that respects Claude Code's /add-dir functionality.
45f9302 to
04a4d07
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (5)
skills/using-skills/find-skills (5)
186-190: Critical: Harden grep invocations against dash-leading patterns.This issue was flagged in a previous review and remains unresolved. Patterns starting with a dash will break these grep commands.
Apply this diff to fix:
if [[ -n "$PATTERN" ]]; then # Pattern mode: search content and paths { - grep -E -r "$PATTERN" "$PROJECT_SKILLS_DIR/" --include="SKILL.md" -l 2>/dev/null || true - find "$PROJECT_SKILLS_DIR/" -name "SKILL.md" -type f 2>/dev/null | grep -E "$PATTERN" 2>/dev/null || true + grep -E -r -e "$PATTERN" --include="SKILL.md" -l "$PROJECT_SKILLS_DIR/" 2>/dev/null || true + find "$PROJECT_SKILLS_DIR/" -name "SKILL.md" -type f 2>/dev/null | grep -E -e "$PATTERN" 2>/dev/null || true } | sort -u
221-224: Critical: Harden grep invocations against dash-leading patterns.Same issue as lines 186-190.
Apply this diff to fix:
if [[ -n "$PATTERN" ]]; then { - grep -E -r "$PATTERN" "$ADDITIONAL_SKILLS_DIR/" --include="SKILL.md" -l 2>/dev/null || true - find "$ADDITIONAL_SKILLS_DIR/" -name "SKILL.md" -type f 2>/dev/null | grep -E "$PATTERN" 2>/dev/null || true + grep -E -r -e "$PATTERN" --include="SKILL.md" -l "$ADDITIONAL_SKILLS_DIR/" 2>/dev/null || true + find "$ADDITIONAL_SKILLS_DIR/" -name "SKILL.md" -type f 2>/dev/null | grep -E -e "$PATTERN" 2>/dev/null || true } | sort -u
246-247: Critical: Correct grep pattern handling.The
--here separates options from arguments but doesn't protect the pattern. The pattern is still positional and vulnerable.Apply this diff to fix:
# Pattern mode: search content and paths (exclude .claude directories) { - grep -E -r -- "$PATTERN" "$SKILLS_DIR/" --include="SKILL.md" --exclude-dir=".claude" -l 2>/dev/null || true - find "$SKILLS_DIR/" -name "SKILL.md" -type f -not -path "*/.claude/*" 2>/dev/null | grep -E -- "$PATTERN" 2>/dev/null || true + grep -E -r -e "$PATTERN" --include="SKILL.md" --exclude-dir=".claude" -l "$SKILLS_DIR/" 2>/dev/null || true + find "$SKILLS_DIR/" -name "SKILL.md" -type f -not -path "*/.claude/*" 2>/dev/null | grep -E -e "$PATTERN" 2>/dev/null || true } | sort -u
212-214: Critical: Use fixed-string matching for shadowing checks.This issue was flagged in a previous review and remains unresolved. Regex matching can misidentify paths containing regex metacharacters (e.g.,
skills/test[1].md), breaking shadowing logic.Apply this diff to fix:
# Skip if already seen (shadowed by current dir) - if echo "$seen_skills_list" | grep -q "^${skill_path}$"; then + if echo "$seen_skills_list" | grep -F -x -q "${skill_path}"; then continue fi
238-239: Critical: Use fixed-string matching for shadowing checks.Same issue as line 212-214.
Apply this diff to fix:
# Skip if shadowed by project skill - echo "$seen_skills_list" | grep -q "^${skill_path}$" && continue + echo "$seen_skills_list" | grep -F -x -q "${skill_path}" && continue
🧹 Nitpick comments (1)
skills/meta/writing-skills/SKILL.md (1)
80-106: Add language specifiers to code blocks.The documentation structure clearly explains the global vs project distinction with helpful examples. However, the code blocks at lines 82-89 and 93-100 should specify a language for proper syntax highlighting.
Apply this diff to add language specifiers:
-**Global skills** (in your branch at `${SUPERPOWERS_SKILLS_ROOT}`): - -``` +**Global skills** (in your branch at `${SUPERPOWERS_SKILLS_ROOT}`): + +```text ${SUPERPOWERS_SKILLS_ROOT}/ skills/ category/ skill-name/ SKILL.md # Main reference (required) supporting-file.* # Only if neededProject skills (version-controlled with project):
-
+text
.claude/
skills/
category/
skill-name/
SKILL.md # Same structure as global
tool/ # Executable scripts if neededBased on static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
skills/meta/writing-skills/SKILL.md(2 hunks)skills/using-skills/SKILL.md(1 hunks)skills/using-skills/find-skills(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- skills/using-skills/SKILL.md
🧰 Additional context used
🪛 LanguageTool
skills/meta/writing-skills/SKILL.md
[grammar] ~56-~56: There might be a mistake here.
Context: ... project skill (.claude/skills/) when:** - Team-specific workflows and conventions ...
(QB_NEW_EN)
[grammar] ~57-~57: There might be a mistake here.
Context: ... Team-specific workflows and conventions - Project architecture patterns - Domain-s...
(QB_NEW_EN)
[grammar] ~58-~58: There might be a mistake here.
Context: ...ventions - Project architecture patterns - Domain-specific best practices - Onboard...
(QB_NEW_EN)
[grammar] ~59-~59: There might be a mistake here.
Context: ...atterns - Domain-specific best practices - Onboarding patterns for this codebase *...
(QB_NEW_EN)
[grammar] ~62-~62: There might be a mistake here.
Context: ...this codebase Don't create skill for: - One-off solutions - Standard practices w...
(QB_NEW_EN)
[grammar] ~63-~63: There might be a mistake here.
Context: ... create skill for:** - One-off solutions - Standard practices well-documented elsew...
(QB_NEW_EN)
[grammar] ~64-~64: There might be a mistake here.
Context: ...dard practices well-documented elsewhere - Simple reminders (use CLAUDE.md instead)...
(QB_NEW_EN)
[grammar] ~102-~102: There might be a mistake here.
Context: ...cripts if needed ``` Key differences: - Global skills: Broadly applicable, perso...
(QB_NEW_EN)
[grammar] ~103-~103: There might be a mistake here.
Context: ...adly applicable, personal working branch - Project skills: Team-shared, version-con...
(QB_NEW_EN)
[grammar] ~104-~104: There might be a mistake here.
Context: ... skills: Team-shared, version-controlled - Current dir project skills shadow global...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.18.1)
skills/meta/writing-skills/SKILL.md
82-82: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
93-93: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (3)
skills/meta/writing-skills/SKILL.md (1)
50-66: LGTM! Clear distinction between global and project skills.The updated guidance provides clear criteria for when to create global vs project skills, and the "Don't create skill for:" section appropriately consolidates anti-patterns.
skills/using-skills/find-skills (2)
59-111: LGTM! Path normalization and deduplication logic is sound.The implementation correctly:
- Resolves relative paths from the settings file location
- Handles absolute paths appropriately
- Excludes the current directory to prevent duplicates
- Deduplicates paths across multiple settings files
268-284: LGTM! Display logic correctly implements location tagging.The output formatting properly distinguishes between:
- Current directory project skills (no location tag)
- Additional directory skills (with project name tag)
- Global skills (no location tag)
The conditional logic ensures clean output whether or not
when_to_useis present.
| # Function to get additional directories from Claude Code settings | ||
| # Outputs: "path|settings_dir" for each additionalDirectory found | ||
| get_additional_dirs() { | ||
| local git_root | ||
| git_root=$(git rev-parse --show-toplevel 2>/dev/null) || return | ||
|
|
||
| local current_dir | ||
| current_dir=$(pwd) | ||
|
|
||
| # Walk up from current dir to git root, checking for settings files | ||
| local check_dir="$current_dir" | ||
| while true; do | ||
| local settings_files=( | ||
| "$check_dir/.claude/settings.json" | ||
| "$check_dir/.claude/settings.local.json" | ||
| ) | ||
|
|
||
| for settings_file in "${settings_files[@]}"; do | ||
| if [[ -f "$settings_file" ]]; then | ||
| # Extract paths from additionalDirectories array only | ||
| # Output format: "path|settings_dir" so we can resolve relative paths correctly | ||
| awk -v settings_dir="$check_dir" ' | ||
| /"additionalDirectories"/ { in_section=1; next } | ||
| in_section && /\]/ { in_section=0; next } | ||
| in_section && /^[[:space:]]*"/ { | ||
| gsub(/^[[:space:]]*"|"[[:space:]]*,?[[:space:]]*$/, "") | ||
| print $0 "|" settings_dir | ||
| } | ||
| ' "$settings_file" 2>/dev/null | ||
| fi | ||
| done | ||
|
|
||
| # Stop if we've reached git root | ||
| if [[ "$(cd "$check_dir" && pwd)" == "$(cd "$git_root" && pwd)" ]]; then | ||
| break | ||
| fi | ||
|
|
||
| # Move up one directory | ||
| check_dir=$(dirname "$check_dir") | ||
|
|
||
| # Safety check | ||
| if [[ "$check_dir" == "/" ]]; then | ||
| break | ||
| fi | ||
| done | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify JSON parsing handles edge cases correctly.
The awk-based JSON parser should handle most simple cases of additionalDirectories arrays. However, it may not correctly handle:
- Escaped quotes within paths (e.g.,
"path/with\"quote") - Multiline strings (unlikely in path arrays but possible)
- Comments in JSON files (not strictly valid but sometimes present)
Run the following script to test edge cases:
🏁 Script executed:
#!/bin/bash
# Test get_additional_dirs with various JSON edge cases
# Create test directory structure
mkdir -p /tmp/test-skill-parsing/.claude
cd /tmp/test-skill-parsing
# Test 1: Normal case
cat > .claude/settings.json <<'EOF'
{
"additionalDirectories": [
"../project1",
"/absolute/path"
]
}
EOF
# Test 2: Edge case with spaces and special characters
cat > .claude/settings.local.json <<'EOF'
{
"additionalDirectories": [
"../project with spaces",
"../project-with-dashes"
]
}
EOF
# Extract the get_additional_dirs function and test it
# (Would need to source the actual script and run the function)
echo "Manual verification needed: Check if paths with spaces, dashes, and special characters are correctly extracted"Length of output: 110
🏁 Script executed:
#!/bin/bash
set -e
# Prepare test under repo root so git rev-parse succeeds
repo_root=$(git rev-parse --show-toplevel)
cd "$repo_root"
# Clean up any prior test
rm -rf tmp_test
# Create nested .claude with edge-case JSON
mkdir -p tmp_test/a/b/c/.claude
cat > tmp_test/a/b/c/.claude/settings.json << 'EOF'
{
"additionalDirectories": [
"../proj1",
"../proj\"quote",
"/abs/path",
"../withcomment" // inline comment
]
}
EOF
# Invoke get_additional_dirs from the script
cd tmp_test/a/b/c
source "$repo_root/skills/using-skills/find-skills"
get_additional_dirsLength of output: 98
Replace AWK-based JSON parsing with jq
AWK fails on escaped quotes, inline comments and other JSON edge cases; a dedicated JSON parser (e.g. jq) guarantees correct extraction of additionalDirectories.
🤖 Prompt for AI Agents
In skills/using-skills/find-skills around lines 12 to 57, the current AWK-based
JSON extraction can break on escaped quotes/comments and other JSON edge cases;
replace the AWK block with a jq-based extraction: for each found settings_file
call jq to safely read .additionalDirectories[] and output each entry joined
with the settings directory as "path|settings_dir"; ensure you handle missing
.additionalDirectories gracefully (no output) and add a fallback error message
if jq is not installed so the function fails clearly.
Summary
Implements project-specific skills that respect Claude Code's
additionalDirectoriesfrom.claude/settings*.jsonfiles.Changes
additionalDirectoriesfrom.claude/settings.jsonand.claude/settings.local.json.claude/skills/(no location tag shown).claude/skills/(shown with project name tag)writing-skills/SKILL.mdfor project skillsBehavior
When running
find-skillsfrom a directory:.claude/skills/: no location tagadditionalDirectories: shown asUse skills/path (project-name) when ...Example
Working in
/pluto/haarlem/with settings file containing"additionalDirectories": ["../pluto"]:Why this approach?
This replaces the hierarchical walk-up approach (PR #10) with context-aware discovery that better aligns with Claude Code's
/add-dirfunctionality. Instead of automatically discovering all.claude/skills/up to git root, it only searches directories explicitly listed inadditionalDirectories, matching how Claude Code manages multi-project contexts.Test plan
Tested with:
../pluto)Summary by CodeRabbit
New Features
Documentation