feat(skills): add /add-skill command for external skill import#135
feat(skills): add /add-skill command for external skill import#135marcusquinn merged 7 commits intomainfrom
Conversation
Implements a complete system for importing skills from external GitHub repositories into aidevops format: - add-skill-helper.sh: Main script for fetching, detecting format, converting, and registering external skills - skill-update-helper.sh: Check imported skills for upstream updates - skill-sources.json: Registry tracking imported skills with metadata - /add-skill command: Slash command definition for AI assistants - add-skill.md subagent: Detailed conversion logic and merge strategies - setup.sh: Added create_skill_symlinks() for cross-tool compatibility Supports multiple input formats (SKILL.md, AGENTS.md, .cursorrules, raw markdown) and provides conflict detection with merge options.
Summary of ChangesHello @marcusquinn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the agent's capabilities by introducing a powerful and flexible system for importing and managing external skills. It streamlines the process of integrating skills from diverse sources, ensuring they are properly formatted, categorized, and kept up-to-date. This feature empowers users to easily extend the agent's knowledge base and functionality, fostering a more dynamic and adaptable AI environment. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds an external-skill import system: a registry schema, import and update helper scripts, docs and tooling for import/update flows, setup integration to create symlinks for imported skills, minor CLI parsing refactors, and a TODO task. setup.sh includes a duplicated Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as add-skill<br/>helper
participant GitHub as GitHub API
participant FS as Local FS
participant Registry as skill-sources.json
User->>CLI: add <github-url>
CLI->>CLI: parse_github_url()
CLI->>GitHub: fetch/clone repo
GitHub-->>CLI: repo files
CLI->>FS: detect_format() / read files
CLI->>CLI: extract metadata / convert
CLI->>FS: check_conflicts()
alt conflict
CLI->>User: prompt resolution (Merge/Replace/Separate/Skip)
end
CLI->>FS: write converted skill
CLI->>Registry: register_skill()
Registry-->>CLI: confirmation
CLI->>User: success (remind run setup.sh)
sequenceDiagram
actor CI_User as User/CI
participant Updater as skill-update<br/>helper
participant Registry as skill-sources.json
participant GitHub as GitHub API
participant Importer as add-skill<br/>helper
CI_User->>Updater: check-updates [--auto-update?]
Updater->>Registry: read skills
loop per skill
Updater->>GitHub: get latest commit
GitHub-->>Updater: latest SHA
Updater->>Updater: compare vs local_commit
alt update available
Updater->>Registry: update_last_checked()
alt auto-update
Updater->>Importer: invoke add-skill to update
end
Updater->>CI_User: report update available
else up-to-date
Updater->>Registry: update_last_checked()
end
end
Updater-->>CI_User: exit code / summary
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Wed Jan 21 03:16:46 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Wed Jan 21 03:17:27 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive system for importing external skills from GitHub, which is a great new feature. The implementation includes helper scripts for adding, listing, updating, and removing skills, along with a tracking mechanism in skill-sources.json. The changes to setup.sh to integrate these imported skills with various AI assistants via symlinks are also well-thought-out.
I've identified a few critical and high-severity issues that should be addressed:
- The
sedfallback inadd-skill-helper.shfor updating the JSON registry is buggy and could corrupt the file. It's better to makejqa hard requirement. - There's a JSON injection vulnerability in
skill-update-helper.shwhen generating JSON output, which could lead to invalid JSON. - The symlink creation for
ampinsetup.shappears to be incorrect and needs a special case.
I've also left some medium-severity comments regarding unimplemented features presented to the user, fragile URL parsing, and ensuring all files (including dotfiles) are copied.
Overall, this is a solid contribution. Addressing these points will make the new feature more robust and reliable.
.agent/scripts/add-skill-helper.sh
Outdated
| # Add to skills array using jq if available, otherwise use sed | ||
| if command -v jq &>/dev/null; then | ||
| local tmp_file | ||
| tmp_file=$(mktemp) | ||
| jq --argjson entry "$new_entry" '.skills += [$entry]' "$SKILL_SOURCES" > "$tmp_file" | ||
| mv "$tmp_file" "$SKILL_SOURCES" | ||
| else | ||
| # Fallback: simple append (less robust) | ||
| log_warning "jq not found, using basic JSON append" | ||
| sed -i.bak 's/"skills": \[/"skills": [\n'"$(echo "$new_entry" | tr '\n' ' ')"',/' "$SKILL_SOURCES" | ||
| rm -f "${SKILL_SOURCES}.bak" | ||
| fi |
There was a problem hiding this comment.
The sed fallback for appending a new skill to skill-sources.json is not robust and can lead to a corrupted JSON file. For example, it adds a trailing comma which is invalid for many JSON parsers, and it doesn't correctly handle cases where the skills array is not empty.
Since jq is a dependency for other critical functions in this script and skill-update-helper.sh, it would be safer to make jq a hard requirement for this script and remove the sed fallback. This will improve the script's reliability and simplify the code.
| # Add to skills array using jq if available, otherwise use sed | |
| if command -v jq &>/dev/null; then | |
| local tmp_file | |
| tmp_file=$(mktemp) | |
| jq --argjson entry "$new_entry" '.skills += [$entry]' "$SKILL_SOURCES" > "$tmp_file" | |
| mv "$tmp_file" "$SKILL_SOURCES" | |
| else | |
| # Fallback: simple append (less robust) | |
| log_warning "jq not found, using basic JSON append" | |
| sed -i.bak 's/"skills": \[/"skills": [\n'"$(echo "$new_entry" | tr '\n' ' ')"',/' "$SKILL_SOURCES" | |
| rm -f "${SKILL_SOURCES}.bak" | |
| fi | |
| # Add to skills array using jq | |
| if ! command -v jq &>/dev/null; then | |
| log_error "jq is required to register skills. Please install jq and try again." | |
| return 1 | |
| fi | |
| local tmp_file | |
| tmp_file=$(mktemp) | |
| jq --argjson entry "$new_entry" '.skills += [$entry]' "$SKILL_SOURCES" > "$tmp_file" | |
| mv "$tmp_file" "$SKILL_SOURCES" |
| echo " Latest: ${latest_commit:0:7}" | ||
| echo "" | ||
| ((updates_available++)) || true | ||
| results+=("{\"name\":\"$name\",\"status\":\"unknown\",\"latest\":\"$latest_commit\"}") |
There was a problem hiding this comment.
When constructing the JSON output for the results array, you are embedding shell variables directly into a JSON string (also on lines 260 and 274). If variables like $name, $current_commit, or $latest_commit contain characters that are special in JSON (like quotes " or backslashes \), it will result in invalid JSON. This is a form of injection vulnerability.
You should use jq to safely construct the JSON objects. For example:
results+=("$(jq -n --arg name "$name" --arg status "unknown" --arg latest "$latest_commit" '{name: $name, status: $status, latest: $latest}')")| # Create symlinks in each AI assistant directory | ||
| for skill_dir in "${skill_dirs[@]}"; do | ||
| local target_dir="$skill_dir/$name" | ||
| local target_file="$target_dir/SKILL.md" | ||
|
|
||
| # Create skill subdirectory | ||
| mkdir -p "$target_dir" 2>/dev/null || continue | ||
|
|
||
| # Create symlink (remove existing first) | ||
| rm -f "$target_file" 2>/dev/null || true | ||
| if ln -sf "$full_path" "$target_file" 2>/dev/null; then | ||
| ((created_count++)) | ||
| fi | ||
| done |
There was a problem hiding this comment.
The symlink creation logic treats all AI assistants the same, creating a subdirectory for each skill (e.g., ~/.config/opencode/skills/my-skill/SKILL.md). However, for amp, the expected structure is likely a flat directory of markdown files (e.g., ~/.config/amp/tools/my-skill.md). The current implementation creates ~/.config/amp/tools/my-skill/SKILL.md, which amp might not recognize.
The documentation in .agent/tools/build-agent/add-skill.md also suggests a flat structure for amp. You should add a special case for amp inside the loop to create the correct symlink structure.
# Create symlinks in each AI assistant directory
for skill_dir in "${skill_dirs[@]}"; do
if [[ "$skill_dir" == *"/amp/tools" ]]; then
# Amp expects flat .md files
local target_file="$skill_dir/$name.md"
rm -f "$target_file" 2>/dev/null || true
if ln -sf "$full_path" "$target_file" 2>/dev/null; then
((created_count++))
fi
else
# Other tools expect skill-name/SKILL.md structure
local target_dir="$skill_dir/$name"
local target_file="$target_dir/SKILL.md"
# Create skill subdirectory
mkdir -p "$target_dir" 2>/dev/null || continue
# Create symlink (remove existing first)
rm -f "$target_file" 2>/dev/null || true
if ln -sf "$full_path" "$target_file" 2>/dev/null; then
((created_count++))
fi
fi
done| input="${input%.git}" | ||
|
|
||
| # Remove /tree/main or /tree/master if present | ||
| input=$(echo "$input" | sed -E 's|/tree/(main|master)(/.*)?$|\2|') |
There was a problem hiding this comment.
The sed command to parse the GitHub URL only handles main and master as branch names when stripping the /tree/... part of a URL. This could fail for repositories that use other branch names like develop. You can make this more generic to handle any branch name.
| input=$(echo "$input" | sed -E 's|/tree/(main|master)(/.*)?$|\2|') | |
| input=$(echo "$input" | sed -E 's|/tree/[^/]+(/.*)?$|\1|') |
| 1) | ||
| log_info "Merging with existing..." | ||
| # TODO: Implement merge logic | ||
| log_warning "Merge not yet implemented, using replace" | ||
| ;; |
There was a problem hiding this comment.
The interactive prompt offers a "Merge" option for conflicts, but the implementation is a TODO and falls back to "Replace". This can be misleading for the user who selects merge but gets a replacement. It would be better to either remove the "Merge" option from the prompt until it's implemented, or at least explicitly inform the user that a replacement will occur and ask for confirmation.
| if [[ -d "$source_dir/$resource_dir" ]]; then | ||
| local target_resource_dir=".agent/${target_path}/$resource_dir" | ||
| mkdir -p "$target_resource_dir" | ||
| cp -r "$source_dir/$resource_dir/"* "$target_resource_dir/" 2>/dev/null || true |
There was a problem hiding this comment.
The command cp -r "$source_dir/$resource_dir/"* ... will not copy hidden files (dotfiles) from the resource directories (scripts, references, assets). If any of these directories could contain dotfiles that are necessary for the skill, they will be missed. A more robust way to copy all contents of a directory is to append /. to the source path.
| cp -r "$source_dir/$resource_dir/"* "$target_resource_dir/" 2>/dev/null || true | |
| cp -r "$source_dir/$resource_dir/." "$target_resource_dir/" 2>/dev/null || true |
🤖 Augment PR SummarySummary: Adds an external-skill import workflow so users can pull “skills” from GitHub repos into aidevops’ local format and keep them trackable for updates. Changes:
Technical Notes: Update checks rely on 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
.agent/scripts/add-skill-helper.sh
Outdated
|
|
||
| # Check for conflicts | ||
| local conflicts | ||
| if conflicts=$(check_conflicts "$target_path" ".agent"); then |
There was a problem hiding this comment.
This if conflicts=$(check_conflicts ...) condition looks inverted: check_conflicts returns non-zero when conflicts exist, so the conflict prompt is skipped and existing skills may be overwritten even without --force. Consider adjusting the control flow so conflicts are handled before writing the target file.
🤖 Was this useful? React with 👍 or 👎
.agent/scripts/add-skill-helper.sh
Outdated
| # openskills handles everything, just register it | ||
| local skill_name="${custom_name:-$(basename "${subpath:-$repo}")}" | ||
| skill_name=$(to_kebab_case "$skill_name") | ||
| register_skill "$skill_name" "https://github.com/$owner/$repo" ".agent/skills/$skill_name" "skill-md" "" "openskills" "Installed via openskills CLI" |
There was a problem hiding this comment.
When openskills install succeeds, register_skill stores .agent/skills/$skill_name (no .md), which doesn’t match the .agent/<...>.md paths used elsewhere and expected by create_skill_symlinks(). That could break later listing/removal/symlink creation for openskills-installed entries.
🤖 Was this useful? React with 👍 or 👎
.agent/scripts/add-skill-helper.sh
Outdated
| local new_entry | ||
| new_entry=$(cat << EOF | ||
| { | ||
| "name": "$name", |
There was a problem hiding this comment.
new_entry is built via string interpolation; if any field contains quotes/newlines (e.g., a future notes value), it can corrupt skill-sources.json and make subsequent jq operations fail. Consider ensuring these values are JSON-escaped (e.g., by constructing the object via jq args).
🤖 Was this useful? React with 👍 or 👎
| # Update all skills with available updates | ||
| log_info "Checking and updating all skills..." | ||
| AUTO_UPDATE=true | ||
| cmd_check |
There was a problem hiding this comment.
In cmd_update (no name), calling cmd_check directly can return 1 when updates are available, and with set -e that will terminate the script before cmd_update can return successfully. This makes the update command look like a failure even when auto-updates ran.
🤖 Was this useful? React with 👍 or 👎
setup.sh
Outdated
| # Create symlinks in each AI assistant directory | ||
| for skill_dir in "${skill_dirs[@]}"; do | ||
| local target_dir="$skill_dir/$name" | ||
| local target_file="$target_dir/SKILL.md" |
There was a problem hiding this comment.
create_skill_symlinks() creates <skill_dir>/<name>/SKILL.md for all destinations, including $HOME/.config/amp/tools, but the new docs in tools/build-agent/add-skill.md describe Amp expecting $HOME/.config/amp/tools/<name>.md. If Amp doesn’t support the directory+SKILL.md layout, imported skills may not be discovered there.
🤖 Was this useful? React with 👍 or 👎
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @.agent/scripts/add-skill-helper.sh:
- Around line 489-493: The script currently resolves target_path via
determine_target_path and logs/uses ".agent/$target_path", causing file ops to
diverge from the registry when AGENTS_DIR/AIDEVOPS_AGENTS_DIR is set; update all
file path usage to prepend the canonical $AGENTS_DIR (or fallback
AIDEVOPS_AGENTS_DIR) instead of hardcoded ".agent/", e.g., replace usages around
target_path, log_info "Target path...", and any create/list/remove/update logic
(including the blocks referenced at lines ~542-602) to operate on
"$AGENTS_DIR/$target_path"; also adjust the conflict check to compare against
files inside $AGENTS_DIR so all operations remain deterministic and consistent
with the registry.
- Around line 494-531: The conflict-check branch is inverted: because
check_conflicts returns non-zero when conflicts exist, the current if-block runs
only when there are no conflicts and can silently overwrite files. Fix by
capturing the output of check_conflicts regardless of its exit code and then
test the captured string (e.g., conflicts=$(check_conflicts "$target_path"
".agent") || true; if [[ -n "$conflicts" ]]; then ... fi), and keep the existing
prompt/choice logic gated by [[ "$force" != true ]]; update references to
check_conflicts, conflicts, target_path, force, log_warning, choice,
to_kebab_case, and determine_target_path accordingly so prompts run only when
conflicts are actually present.
- Around line 291-329: The YAML frontmatter can be broken by unescaped
characters in name/description; update convert_skill_md to escape or quote these
values before writing the header: when generating the header for "$target_file"
(the block that writes --- description: ... mode: ... and #
${name:-$skill_name}) ensure you handle ${description} and ${name} safely by
either emitting description as a YAML block scalar (use "description: |"
followed by an indented, newline-escaped ${description}) or by producing a
properly quoted/escaped scalar (escape backslashes and quotes) and similarly
quote/escape ${name} (or wrap it in a YAML-safe scalar) so special characters
(quotes, colons, backslashes, newlines) do not break the frontmatter; replace
the current direct interpolation with this escaped/quoted output when writing to
"$target_file".
- Around line 331-377: The sed-based fallback in register_skill() is brittle and
must be removed; update register_skill to require jq and hard-fail if jq is not
available (consistent with cmd_check_updates() and cmd_remove_skill()), i.e.,
check command -v jq and if missing call log_error/exit with a clear message
instead of using sed, and keep the existing jq-based flow that uses jq --argjson
entry "$new_entry" '.skills += [$entry]' "$SKILL_SOURCES" to update
SKILL_SOURCES safely; refer to the register_skill() function and the jq usage
there when making the change.
In @.agent/scripts/skill-update-helper.sh:
- Around line 154-176: The get_latest_commit function's curl call lacks a
timeout and can hang; update the curl invocation in get_latest_commit to include
connection and overall timeouts (e.g., add --connect-timeout 5 and --max-time 10
or -m 10) while keeping existing flags (-s and the Accept header) so the call
fails fast on unresponsive GitHub API and the function returns error as
intended; ensure the rest of the function still reads the response and handles
empty/null commit as before.
🧹 Nitpick comments (9)
.agent/scripts/commands/add-skill.md (1)
77-83: Minor: Capitalize "Markdown" as a proper noun.Per static analysis, "markdown" should be "Markdown" when referring to the formatting language.
📝 Suggested fix
-| .cursorrules | Cursor | Wrapped in markdown with frontmatter | +| .cursorrules | Cursor | Wrapped in Markdown with frontmatter |.agent/tools/build-agent/add-skill.md (3)
49-68: Fix fenced code block formatting.The nested code block example has markdown formatting issues that may affect rendering. Add blank lines around fenced code blocks and ensure the inner block has a language specifier.
📝 Suggested fix for code block formatting
**Structure:** + ```markdown --- name: skill-name description: One sentence describing when to use this skill --- # Skill Title Instructions for the AI agent... ## How It Works 1. Step one 2. Step two ## Usage + ```bash command examples
151-161: Minor: Capitalize "GitHub" and "GitLab" as proper nouns.Per static analysis and branding guidelines, these should be capitalized.
📝 Suggested fix
-| git, github, gitlab | `tools/git/` | +| git, GitHub, GitLab | `tools/git/` |
258-264: Add blank line before fenced code block.For consistent markdown rendering, add a blank line before the code block in the troubleshooting section.
📝 Suggested fix
Install jq for full functionality: + ```bash brew install jq # macOS apt install jq # Ubuntu/Debian</details> </blockquote></details> <details> <summary>.agent/scripts/skill-update-helper.sh (2)</summary><blockquote> `178-193`: **Add error handling for jq/mv operations.** If `jq` fails or `mv` fails, the skill-sources.json could be left in an inconsistent state. Consider adding error checks. <details> <summary>🔧 Suggested improvement</summary> ```diff update_last_checked() { local skill_name="$1" local timestamp timestamp=$(date -u +"%Y-%m-%dT%H:%M:%SZ") local tmp_file tmp_file=$(mktemp) - jq --arg name "$skill_name" --arg ts "$timestamp" \ + if ! jq --arg name "$skill_name" --arg ts "$timestamp" \ '.skills = [.skills[] | if .name == $name then .last_checked = $ts else . end]' \ - "$SKILL_SOURCES" > "$tmp_file" - - mv "$tmp_file" "$SKILL_SOURCES" + "$SKILL_SOURCES" > "$tmp_file"; then + rm -f "$tmp_file" + log_warning "Failed to update last_checked for $skill_name" + return 1 + fi + + mv "$tmp_file" "$SKILL_SOURCES" || { + rm -f "$tmp_file" + log_warning "Failed to write skill-sources.json" + return 1 + } return 0 }
262-270: Verify add-skill-helper.sh exists before calling.The script assumes
$ADD_SKILL_HELPERexists but doesn't verify. If the helper script is missing, the auto-update will fail silently (error suppressed by the if condition).🔧 Suggested improvement
Add a check at the start of
cmd_checkorcmd_update:+# Verify add-skill-helper exists (needed for auto-update) +if [[ "$AUTO_UPDATE" == true ]] && [[ ! -x "$ADD_SKILL_HELPER" ]]; then + log_error "add-skill-helper.sh not found at $ADD_SKILL_HELPER" + log_info "Cannot auto-update without the helper script" + exit 1 +fi.agent/scripts/add-skill-helper.sh (3)
428-431: Usemktemp+trapfor temp dir safetyA fixed temp dir can collide across concurrent runs, and early returns leave residue. Consider per-run temp dirs with a cleanup trap. As per coding guidelines, prioritize robustness in automation scripts.
♻️ Suggested update
- rm -rf "$TEMP_DIR" - mkdir -p "$TEMP_DIR" + TEMP_DIR="$(mktemp -d "${TMPDIR:-/tmp}/aidevops-skill-import.XXXXXX")" + trap 'rm -rf "$TEMP_DIR"' EXIT @@ - rm -rf "$TEMP_DIR"Also applies to: 533-540, 606-607
674-678: Add curl timeouts for network resilienceThis network call to GitHub API can hang indefinitely without a timeout, freezing the CLI. The
--max-time 10flag ensures responsive behavior and aligns with robustness requirements.♻️ Suggested update
- latest_commit=$(curl -s "$api_url" | jq -r '.[0].sha // empty' 2>/dev/null) + latest_commit=$(curl -fsSL --max-time 10 "$api_url" | jq -r '.[0].sha // empty' 2>/dev/null)
118-151: Support arbitrary/tree/<branch>URLsThe current implementation only strips
mainandmasterbranch segments. Other branch names (e.g.,develop,release-*, feature branches) will leave the/tree/<branch>segment intact, causing subpath mis-parsing. Update the sed pattern to handle any branch name:♻️ Suggested update
- input=$(echo "$input" | sed -E 's|/tree/(main|master)(/.*)?$|\2|') + input=$(echo "$input" | sed -E 's|/tree/[^/]+(/.*)?$|\1|')
| # Determine target path | ||
| local target_path | ||
| target_path=$(determine_target_path "$skill_name" "$description" "$source_dir") | ||
| log_info "Target path: .agent/$target_path" | ||
|
|
There was a problem hiding this comment.
Unify skill storage paths with $AGENTS_DIR
The registry lives under $AGENTS_DIR, but files are written to .agent/ relative paths. If the script is run outside the repo (or AIDEVOPS_AGENTS_DIR is set), list/remove/update can drift. Align all file operations to $AGENTS_DIR (and update the conflict check accordingly). As per coding guidelines, keep file operations deterministic.
🐛 Proposed fix (partial)
- log_info "Target path: .agent/$target_path"
+ log_info "Target path: $AGENTS_DIR/$target_path"
@@
- target_dir=".agent/$(dirname "$target_path")"
+ target_dir="$AGENTS_DIR/$(dirname "$target_path")"
@@
- local target_file=".agent/${target_path}.md"
+ local target_file="$AGENTS_DIR/${target_path}.md"
@@
- local target_resource_dir=".agent/${target_path}/$resource_dir"
+ local target_resource_dir="$AGENTS_DIR/${target_path}/$resource_dir"
@@
- register_skill "$skill_name" "https://github.com/$owner/$repo${subpath:+/$subpath}" ".agent/${target_path}.md" "$format" "$commit_hash" "added" ""
+ register_skill "$skill_name" "https://github.com/$owner/$repo${subpath:+/$subpath}" "$AGENTS_DIR/${target_path}.md" "$format" "$commit_hash" "added" ""Also applies to: 542-602
🤖 Prompt for AI Agents
In @.agent/scripts/add-skill-helper.sh around lines 489 - 493, The script
currently resolves target_path via determine_target_path and logs/uses
".agent/$target_path", causing file ops to diverge from the registry when
AGENTS_DIR/AIDEVOPS_AGENTS_DIR is set; update all file path usage to prepend the
canonical $AGENTS_DIR (or fallback AIDEVOPS_AGENTS_DIR) instead of hardcoded
".agent/", e.g., replace usages around target_path, log_info "Target path...",
and any create/list/remove/update logic (including the blocks referenced at
lines ~542-602) to operate on "$AGENTS_DIR/$target_path"; also adjust the
conflict check to compare against files inside $AGENTS_DIR so all operations
remain deterministic and consistent with the registry.
- Fix conflict check inversion (check_conflicts returns 1 on conflict) - Remove sed fallback, require jq for JSON manipulation - Escape YAML frontmatter values to prevent invalid headers - Add timeout to GitHub API curl calls (10s connect, 30s max) - Fix openskills path registration to include .md extension - Use jq for JSON construction to properly escape special chars - Fix cmd_update set -e issue by handling expected return code - Fix Amp symlink format (expects name.md not name/SKILL.md)
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Wed Jan 21 03:34:24 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
- Use named variable for case statement argument parsing - Fix empty code block in add-skill.md documentation
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Wed Jan 21 03:48:35 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
- Fix beads-sync-helper.sh option parsing - Fix domain-research-helper.sh option parsing - Reduces violations from 152 to 150 (threshold)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.agent/scripts/add-skill-helper.sh:
- Around line 448-462: Openskills installs the skill to
~/.config/opencode/skills/<name>/SKILL.md but the script registers
.agent/skills/${skill_name}.md, causing path mismatch; after a successful
openskills install (inside the openskills success branch), locate the installed
file (~/.config/opencode/skills/${skill_name}/SKILL.md), copy it into
.agent/skills/${skill_name}.md (create .agent/skills if needed), then call
register_skill with the copied .agent/skills/${skill_name}.md path so cmd_list,
cmd_remove and create_skill_symlinks continue to work; keep the code changes
around the openskills branch and update references to register_skill,
create_skill_symlinks, cmd_list, and cmd_remove accordingly.
♻️ Duplicate comments (3)
.agent/scripts/add-skill-helper.sh (2)
531-535: Merge option is a TODO that falls back to replace without explicit confirmation.When user selects option 1 (Merge), the script logs "Merge not yet implemented, using replace" but proceeds without re-confirming. This could surprise users who expect merge behavior.
🔧 Suggested improvement
1) log_info "Merging with existing..." # TODO: Implement merge logic - log_warning "Merge not yet implemented, using replace" + log_warning "Merge not yet implemented. Falling back to replace." + read -rp "Continue with replace? [y/N]: " confirm + if [[ ! "$confirm" =~ ^[Yy]$ ]]; then + log_info "Import cancelled" + return 0 + fi ;;
561-567: Target paths use hardcoded.agent/instead of$AGENTS_DIR.File operations at lines 563, 567, 607, and 621 use
.agent/relative paths, but$AGENTS_DIRmay point elsewhere (e.g., whenAIDEVOPS_AGENTS_DIRis set). This inconsistency can cause drift between the registry and actual file locations.🐛 Proposed fix to use $AGENTS_DIR consistently
# Create target directory local target_dir - target_dir=".agent/$(dirname "$target_path")" + target_dir="$AGENTS_DIR/$(dirname "$target_path")" mkdir -p "$target_dir" # Convert and copy files - local target_file=".agent/${target_path}.md" + local target_file="$AGENTS_DIR/${target_path}.md"Apply similar changes to lines 554, 556, 607, and update the
register_skillcall at line 621 to use$AGENTS_DIRprefix..agent/scripts/skill-update-helper.sh (1)
253-275: JSON output construction is vulnerable to injection from skill metadata.The
resultsarray entries are built via string interpolation (e.g.,"{\"name\":\"$name\",...}"). If$name,$current_commit, or$latest_commitcontain JSON-special characters (quotes, backslashes), the output will be malformed or exploitable.🐛 Proposed fix using jq for safe JSON construction
- results+=("{\"name\":\"$name\",\"status\":\"unknown\",\"latest\":\"$latest_commit\"}") + results+=("$(jq -n --arg name "$name" --arg status "unknown" --arg latest "$latest_commit" \ + '{name: $name, status: $status, latest: $latest}')")Apply similar changes at lines 261 and 275:
# Line 261 results+=("$(jq -n --arg name "$name" --arg status "update_available" \ --arg current "$current_commit" --arg latest "$latest_commit" \ '{name: $name, status: $status, current: $current, latest: $latest}')") # Line 275 results+=("$(jq -n --arg name "$name" --arg status "up_to_date" \ --arg commit "$current_commit" \ '{name: $name, status: $status, commit: $commit}')")
🧹 Nitpick comments (6)
.agent/tools/build-agent/add-skill.md (1)
50-70: Minor markdown formatting: add blank lines around fenced code blocks.Per markdownlint MD031, fenced code blocks should be surrounded by blank lines for consistent rendering across markdown processors.
📝 Suggested fix for markdown formatting
**Structure:** + ```markdown --- name: skill-nameApply similar fixes at lines 83 and 261.
.agent/scripts/add-skill-helper.sh (4)
133-134: Branch name parsing is limited tomainandmaster.The sed expression only strips
/tree/mainor/tree/masterfrom URLs. Repositories using other default branches (e.g.,develop,trunk) will have the branch path retained, potentially causing parse failures.🔧 Proposed fix to handle any branch name
- # Remove /tree/main or /tree/master if present - input=$(echo "$input" | sed -E 's|/tree/(main|master)(/.*)?$|\2|') + # Remove /tree/<branch> suffix if present (handles any branch name) + input=$(echo "$input" | sed -E 's|/tree/[^/]+(/.*)?$|\1|')Note: The
skill-update-helper.shat line 148 already uses the more generic pattern.
307-323: YAML escaping improvements look good, but colon handling may over-escape.The escaping at line 309 replaces
:with:which could produce:(double space) if the colon is already followed by a space. Consider a more targeted replacement.🔧 Suggested refinement for colon escaping
- safe_description=$(printf '%s' "${description:-Imported skill}" | sed 's/\\/\\\\/g; s/"/\\"/g; s/:/: /g; s/^- /\\- /') + # Quote the entire value to avoid YAML parsing issues with special chars + safe_description=$(printf '%s' "${description:-Imported skill}" | sed 's/\\/\\\\/g; s/"/\\"/g')Since the description is already wrapped in double quotes at line 318 (
description: "${safe_description}"), most YAML special characters are handled by the quoting.
604-612: Resource copy excludes dotfiles (hidden files).The glob
"$source_dir/$resource_dir/"*does not match dotfiles. If skills include hidden configuration files inscripts/,references/, orassets/, they will be silently omitted.🔧 Proposed fix to copy all contents including dotfiles
for resource_dir in scripts references assets; do if [[ -d "$source_dir/$resource_dir" ]]; then local target_resource_dir=".agent/${target_path}/$resource_dir" mkdir -p "$target_resource_dir" - cp -r "$source_dir/$resource_dir/"* "$target_resource_dir/" 2>/dev/null || true + cp -r "$source_dir/$resource_dir/." "$target_resource_dir/" 2>/dev/null || true log_success "Copied: $resource_dir/" fi done
662-720:cmd_check_updatesduplicates logic fromskill-update-helper.sh.This function largely mirrors
cmd_checkinskill-update-helper.sh. Consider delegating to the dedicated helper script to avoid maintenance drift between implementations.# Alternative: delegate to skill-update-helper.sh cmd_check_updates() { if [[ -x "$AGENTS_DIR/scripts/skill-update-helper.sh" ]]; then "$AGENTS_DIR/scripts/skill-update-helper.sh" check else log_error "skill-update-helper.sh not found" return 1 fi }.agent/scripts/skill-update-helper.sh (1)
289-298: Final JSON output also uses string interpolation.The summary JSON block (lines 292-297) manually constructs JSON. While the counters are integers and safe, the
resultsarray join could contain malformed entries if the earlier injection issue isn't fixed.🔧 Proposed fix for structured JSON output
# JSON output if requested if [[ "$JSON_OUTPUT" == true ]]; then echo "" - echo "{" - echo " \"up_to_date\": $up_to_date," - echo " \"updates_available\": $updates_available," - echo " \"check_failed\": $check_failed," - echo " \"results\": [$(IFS=,; echo "${results[*]}")]" - echo "}" + jq -n \ + --argjson up_to_date "$up_to_date" \ + --argjson updates_available "$updates_available" \ + --argjson check_failed "$check_failed" \ + --argjson results "$(printf '%s\n' "${results[@]}" | jq -s '.')" \ + '{up_to_date: $up_to_date, updates_available: $updates_available, check_failed: $check_failed, results: $results}' fi
| # Try to use openskills if available | ||
| if command -v openskills &>/dev/null; then | ||
| log_info "Using openskills to fetch skill..." | ||
| if openskills install "$owner/$repo${subpath:+/$subpath}" --yes --universal 2>/dev/null; then | ||
| log_success "Skill installed via openskills" | ||
| # openskills handles everything, just register it | ||
| local skill_name="${custom_name:-$(basename "${subpath:-$repo}")}" | ||
| skill_name=$(to_kebab_case "$skill_name") | ||
| # openskills installs to ~/.config/opencode/skills/<name>/SKILL.md | ||
| # Register with .md extension for consistency with other paths | ||
| register_skill "$skill_name" "https://github.com/$owner/$repo" ".agent/skills/${skill_name}.md" "skill-md" "" "openskills" "Installed via openskills CLI" | ||
| return 0 | ||
| fi | ||
| log_warning "openskills failed, falling back to direct fetch" | ||
| fi |
There was a problem hiding this comment.
Openskills registration path lacks .md extension, causing inconsistency.
When openskills succeeds, the registered path is .agent/skills/${skill_name}.md (line 458), but openskills actually installs to ~/.config/opencode/skills/<name>/SKILL.md (a directory, not a single file). This path mismatch will break cmd_list, cmd_remove, and create_skill_symlinks() in setup.sh.
🐛 Proposed fix to align paths
# openskills installs to ~/.config/opencode/skills/<name>/SKILL.md
- # Register with .md extension for consistency with other paths
- register_skill "$skill_name" "https://github.com/$owner/$repo" ".agent/skills/${skill_name}.md" "skill-md" "" "openskills" "Installed via openskills CLI"
+ # Register the actual openskills installation path
+ register_skill "$skill_name" "https://github.com/$owner/$repo" "$HOME/.config/opencode/skills/${skill_name}/SKILL.md" "skill-md" "" "openskills" "Installed via openskills CLI"Alternatively, copy the skill to .agent/ after openskills install for consistency.
🤖 Prompt for AI Agents
In @.agent/scripts/add-skill-helper.sh around lines 448 - 462, Openskills
installs the skill to ~/.config/opencode/skills/<name>/SKILL.md but the script
registers .agent/skills/${skill_name}.md, causing path mismatch; after a
successful openskills install (inside the openskills success branch), locate the
installed file (~/.config/opencode/skills/${skill_name}/SKILL.md), copy it into
.agent/skills/${skill_name}.md (create .agent/skills if needed), then call
register_skill with the copied .agent/skills/${skill_name}.md path so cmd_list,
cmd_remove and create_skill_symlinks continue to work; keep the code changes
around the openskills branch and update references to register_skill,
create_skill_symlinks, cmd_list, and cmd_remove accordingly.
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Wed Jan 21 03:53:47 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
- SC2016: Add shellcheck disable for intentional single quotes (JSON keys) - IFS: Fix IFS variable scope in add-skill-helper.sh parse_github_url - IFS: Fix IFS variable scope in skill-update-helper.sh JSON output - MD031: Add blank lines before fenced code blocks in add-skill.md
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Wed Jan 21 04:17:48 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
- Use printf for array joining instead of IFS manipulation - Add blank line before code block in add-skill.md
|
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Wed Jan 21 04:24:31 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
1 participant
Summary
Implements a complete system for importing skills from external GitHub repositories into aidevops format.
Changes
New Files
.agent/configs/skill-sources.json- Registry for tracking imported skills with upstream metadata.agent/scripts/add-skill-helper.sh(~630 lines) - Main script for:.agent/scripts/skill-update-helper.sh(~280 lines) - Update checker for:--jsonoutput for scripting.agent/scripts/commands/add-skill.md- Slash command definition.agent/tools/build-agent/add-skill.md- Subagent with detailed conversion logicModified Files
setup.sh- Addedcreate_skill_symlinks()function (~70 lines) to create symlinks from.agent/to:~/.config/opencode/skills/~/.codex/skills/~/.claude/skills/~/.config/amp/tools/Usage
Testing
Quality
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.