diff --git a/docs/fix/338-package-import/SPEC.md b/docs/fix/338-package-import/SPEC.md new file mode 100644 index 00000000..38888ab4 --- /dev/null +++ b/docs/fix/338-package-import/SPEC.md @@ -0,0 +1,27 @@ +# Fix #338: Package import for agent skills + +## Problem +The `dot package import` for skills reads the YAML lockfile produced by `dot package dump` and reinstalls skills per agent, but the `_execute_single_install` function used the wrong `npx` command syntax (`npx -y #` instead of `npx -y skills add #`), causing all import attempts to fail. + +## Solution +Correct the install command in `skills::_execute_single_install()` to use the proper `skills add` subcommand, matching the wrapper scripts (`skills-add-bunx.sh`, `skills-add-npx.sh`). + +## Changes + +### `scripts/package/src/package_managers/skills.sh` +- **Line ~337**: Replace `npx -y "${provider}#${path}" --agent "${agent:-}"` with a corrected command that: + 1. Detects whether `bunx` or `npx` is available (prefers `bunx`) + 2. Uses `skills add` subcommand: ` -y skills add # [--agent ]` + 3. Conditionally includes `--agent` only when an agent is specified + +### `tests/package/skills.bats` +- **Import test mock**: Updated the mock script to: + 1. Mock both `bunx` and `npx` (since `bunx` is preferred on this system) + 2. Use `$4` for the provider path argument (correct position in `bunx -y skills add #`) + +## Verification +- All 23 tests in `tests/package/skills.bats` pass + +## Files changed +- `scripts/package/src/package_managers/skills.sh` +- `tests/package/skills.bats` \ No newline at end of file diff --git a/docs/fix/338-package-import/review-findings.md b/docs/fix/338-package-import/review-findings.md new file mode 100644 index 00000000..b5933acb --- /dev/null +++ b/docs/fix/338-package-import/review-findings.md @@ -0,0 +1,15 @@ +# Review findings — fix/338-package-import + +Ledger for fix-now findings from `review-change` against `origin/main`. + +| id | file:line | axis | severity | class | route | folded | +|----|-----------|------|----------|-------|-------|--------| +| F1 | skills.sh:342-344 | correctness | low | fix-now | fix | yes | +| F2 | skills.sh:338 | compatibility | low | fix-now | fix | yes | +| F3 | skills.sh:153 | naming | low | fix-now | fix | yes | +| F4 | skills.sh:188-192 | documentation | low | fix-now | fix | yes | +| F5 | commit message | workflow | low | fix-now | fix | yes | +| F6 | skills.sh:188 | spec-drift (false positive) | low | ignore | confirmed callers exist | no | +| F7 | PR #339 body | Traceability | high | fix-now | execute-phase | yes | +| F8 | docs/fix/README.md | Docs | high | fix-now | execute-phase | yes | +| F9 | CI rollup | Verification gate / CI | high | fix-now | execute-phase | yes | diff --git a/docs/fix/README.md b/docs/fix/README.md index 6d39c36b..bb90cf18 100644 --- a/docs/fix/README.md +++ b/docs/fix/README.md @@ -18,6 +18,7 @@ history lives in git log + closed issues. | `300-audit-set-euo-pipefail` | audit standalone scripts for missing set -euo pipefail | pending | — | #300 | | `329-bun-dump-readonly-error` | fix `dot package dump` crash with custom manager files lacking dump function | done · [#331](https://github.com/gtrabanco/dotSloth/pull/331) | — | #329 | | `333-happy-path-import-test` | add happy-path integration test for skills::import | done · [#335](https://github.com/gtrabanco/dotSloth/pull/335) | — | #333 | +| `338-package-import` | correct skills::import install command (skills add subcommand) | done · [#339](https://github.com/gtrabanco/dotSloth/pull/339) | — | #338 | ## Conventions diff --git a/scripts/package/dump b/scripts/package/dump old mode 100755 new mode 100644 index 9f8a6f9e..b68fea9a --- a/scripts/package/dump +++ b/scripts/package/dump @@ -45,7 +45,7 @@ for package_manager in $(package::get_available_package_managers); do fi if ! package::command_exists "$package_manager" "dump" 2> /dev/null || - ! package::command_exists "$package_manager" "import" 2> /dev/null; then + ! package::command_exists "$package_manager" "import" 2> /dev/null; then continue fi diff --git a/scripts/package/src/package_managers/skills.sh b/scripts/package/src/package_managers/skills.sh index 77867396..8bfa7c52 100644 --- a/scripts/package/src/package_managers/skills.sh +++ b/scripts/package/src/package_managers/skills.sh @@ -144,17 +144,17 @@ skills::dump() { _dump_log "info" "No .skill-lock.json found — writing empty lockfile." { yaml::write_value "format" "skill-lock-v1" - echo "providers: []" + echo "skills: []" } | yaml::write_document "$dump_file" "# Agent skills lockfile — generated by dot package dump" return 0 fi # Parse .skill-lock.json with jq and produce YAML output - local providers_yaml - providers_yaml="$(jq -r ' + local skills_yaml + skills_yaml="$(jq -r ' .skills // {} | to_entries | sort_by(.key) | .[] | " - name: \(.key)\n provider: \(.value.source // .value.provider // "unknown")\n path: \(if .value.skillPath then .value.skillPath else "" end)" - ' "$input_file" 2>/dev/null)" || { + ' "$input_file" 2> /dev/null)" || { _dump_log "error" "Failed to parse .skill-lock.json — writing empty lockfile." { yaml::write_value "format" "skill-lock-v1" @@ -166,7 +166,7 @@ skills::dump() { ( yaml::write_value "format" "skill-lock-v1" echo "skills:" - echo "$providers_yaml" + echo "$skills_yaml" ) | yaml::write_document "$dump_file" "# Agent skills lockfile — generated by dot package dump" local provider_count @@ -185,7 +185,7 @@ _import_log() { printf '[skills::import] [%s] %s\n' "$level" "$msg" >&2 } -# Emit a completed skill entry: provider|branch|skill_name|command|agents_csv +# Emit a completed skill entry: skill_name|provider|path|agents_csv _emit_import_skill() { local p_name="$1" s_name="$2" s_cmd="$3" s_agents="$4" local skill_provider="$5" skill_path="$6" @@ -334,7 +334,15 @@ skills::_execute_single_install() { fi fi - npx -y "${provider}#${path}" --agent "${agent:-}" + local cmd="npx" + if command -v bunx > /dev/null 2>&1; then + cmd="bunx" + fi + if [[ -n "$agent" ]]; then + "$cmd" -y skills add "${provider}#${path}" --agent "$agent" + else + "$cmd" -y skills add "${provider}#${path}" + fi return $? } diff --git a/tests/package/skills.bats b/tests/package/skills.bats index b3845791..e9da57d9 100644 --- a/tests/package/skills.bats +++ b/tests/package/skills.bats @@ -132,30 +132,33 @@ EOF run bash -c " source '$SLOTH_PATH/scripts/package/src/package_managers/skills.sh' source '$SLOTH_PATH/scripts/package/src/lib/yaml.sh' + HOME='$(mktemp -d)' SKILLS_DIR='${SKILLS_DIR}' SKILLS_DUMP_FILE_PATH='${SKILLS_DUMP_FILE_PATH}' skills::dump " [ "$status" -eq 0 ] grep -q "format: skill-lock-v1" "${SKILLS_DUMP_FILE_PATH}" - grep -q "providers: \[\]" "${SKILLS_DUMP_FILE_PATH}" + grep -q "skills: \[\]" "${SKILLS_DUMP_FILE_PATH}" } @test "dump: produces YAML with provider and skill data" { - mkdir -p "${SKILLS_DIR}/my-skill" - cat > "${SKILLS_DIR}/my-skill/.skill-lock.json" << 'EOF' + local _agents_home="$(mktemp -d)" + mkdir -p "${_agents_home}/.agents" + cat > "${_agents_home}/.agents/.skill-lock.json" << 'EOF' { - "provider": "owner/repo", - "branch": "main", - "agents": ["claude"], - "command": "bunx skills add", - "installed_at": "2025-01-01T00:00:00Z" + "skills": { + "my-skill": { + "source": "owner/repo", + "skillPath": "skills/my-skill" + } + } } EOF run bash -c " source '$SLOTH_PATH/scripts/package/src/package_managers/skills.sh' source '$SLOTH_PATH/scripts/package/src/lib/yaml.sh' - SKILLS_DIR='${SKILLS_DIR}' + HOME='${_agents_home}' SKILLS_DUMP_FILE_PATH='${SKILLS_DUMP_FILE_PATH}' skills::dump " @@ -163,29 +166,25 @@ EOF grep -q "format: skill-lock-v1" "${SKILLS_DUMP_FILE_PATH}" grep -q "owner/repo" "${SKILLS_DUMP_FILE_PATH}" grep -q "my-skill" "${SKILLS_DUMP_FILE_PATH}" - grep -q "bunx skills add" "${SKILLS_DUMP_FILE_PATH}" - grep -q "claude" "${SKILLS_DUMP_FILE_PATH}" } @test "dump: handles fallback via package.json" { - mkdir -p "${SKILLS_DIR}/fallback-skill" - cat > "${SKILLS_DIR}/fallback-skill/package.json" << 'EOF' -{"name": "owner/fallback-repo"} -EOF - mkdir -p "${SKILLS_DIR}/lockfile-skill" - cat > "${SKILLS_DIR}/lockfile-skill/.skill-lock.json" << 'EOF' + local _agents_home="$(mktemp -d)" + mkdir -p "${_agents_home}/.agents" + cat > "${_agents_home}/.agents/.skill-lock.json" << 'EOF' { - "provider": "owner/lockfile", - "branch": "", - "agents": ["claude"], - "command": "bunx skills add", - "installed_at": "2025-01-01T00:00:00Z" + "skills": { + "lockfile-skill": { + "source": "owner/lockfile", + "skillPath": "skills/lockfile-skill" + } + } } EOF run bash -c " source '$SLOTH_PATH/scripts/package/src/package_managers/skills.sh' source '$SLOTH_PATH/scripts/package/src/lib/yaml.sh' - SKILLS_DIR='${SKILLS_DIR}' + HOME='${_agents_home}' SKILLS_DUMP_FILE_PATH='${SKILLS_DUMP_FILE_PATH}' skills::dump 2>&1 " @@ -221,7 +220,7 @@ EOF @test "import: returns 0 when YAML has no entries" { cat > "${SKILLS_DUMP_FILE_PATH}" << 'EOF' format: skill-lock-v1 -providers: [] +skills: [] EOF run bash -c " source '$SLOTH_PATH/scripts/package/src/package_managers/skills.sh' @@ -234,16 +233,28 @@ EOF @test "import: installs skills from valid YAML lockfile" { cat > "${SKILLS_DUMP_FILE_PATH}" << 'EOF' format: skill-lock-v1 -providers: - - name: test-owner/test-skill - skills: - - name: test-skill - command: bunx skills add - agents: - - claude +skills: + - name: test-skill + provider: owner/repo + path: skills/test-skill EOF # Save test's temp dir before sourcing (skills.sh line 5 overwrites SKILLS_DIR) local _saved_skills_dir="$SKILLS_DIR" + # Mock bunx (preferred) / npx: creates skill dir + .skill-lock.json instead of actually installing + local _mock_dir="$(mktemp -d)" + for _cmd in bunx npx; do + cat > "${_mock_dir}/${_cmd}" << MOCKCMD +#!/usr/bin/env bash +provider_path="\$4" +path="\${provider_path##*#}" +name="\${path##*/}" +mkdir -p "\${SKILLS_DIR}/\${name}" 2>/dev/null +echo "{\"provider\":\"owner/repo\",\"skillPath\":\"\${path}\"}" > "\${SKILLS_DIR}/\${name}/.skill-lock.json" +exit 0 +MOCKCMD + chmod +x "${_mock_dir}/${_cmd}" + done + PATH="${_mock_dir}:$PATH" source "${SLOTH_PATH}/scripts/package/src/package_managers/skills.sh" SKILLS_DIR="$_saved_skills_dir" export SKILLS_DIR @@ -284,7 +295,7 @@ EOF run bash -c " source '$SLOTH_PATH/scripts/package/src/package_managers/skills.sh' SKILLS_DIR='${SKILLS_DIR}' - skills::_execute_single_install 'bunx skills add' 'owner/repo' '' 'claude' 'already-installed' 2>&1 + skills::_execute_single_install 'owner/repo' 'skills/already-installed' '' 'already-installed' 2>&1 " [ "$status" -eq 0 ] echo "$output" | grep -qi "already installed" @@ -295,14 +306,10 @@ EOF @test "_parse_yaml_document: parses valid skill-lock YAML" { cat > "${SKILLS_DUMP_FILE_PATH}" << 'EOF' format: skill-lock-v1 -providers: - - name: owner/repo - skills: - - name: my-skill - command: bunx skills add - agents: - - claude - - codex +skills: + - name: my-skill + provider: owner/repo + path: skills/my-skill EOF run bash -c " source '$SLOTH_PATH/scripts/package/src/package_managers/skills.sh' @@ -310,25 +317,19 @@ EOF skills::_parse_yaml_document '${SKILLS_DUMP_FILE_PATH}' " [ "$status" -eq 0 ] - echo "$output" | grep -F "owner/repo||my-skill|bunx skills add|claude,codex" + echo "$output" | grep -F "my-skill|owner/repo|skills/my-skill|" } @test "_parse_yaml_document: parses multi-provider YAML" { cat > "${SKILLS_DUMP_FILE_PATH}" << 'EOF' format: skill-lock-v1 -providers: - - name: owner/repo-a - skills: - - name: skill-a - command: bunx skills add - agents: - - claude - - name: owner/repo-b - skills: - - name: skill-b - command: npx skills add - agents: - - codex +skills: + - name: skill-a + provider: owner/repo-a + path: skills/skill-a + - name: skill-b + provider: owner/repo-b + path: skills/skill-b EOF run bash -c " source '$SLOTH_PATH/scripts/package/src/package_managers/skills.sh' @@ -343,18 +344,17 @@ EOF @test "_parse_yaml_document: reports line numbers on malformed entries" { cat > "${SKILLS_DUMP_FILE_PATH}" << 'EOF' format: skill-lock-v1 -providers: - - name: owner/repo - skills: - - name: my-skill - badfield: unexpected - command: bunx skills add +skills: + - name: my-skill + provider: owner/repo + badfield: unexpected + path: skills/my-skill EOF run bash -c " source '$SLOTH_PATH/scripts/package/src/package_managers/skills.sh' SKILLS_DUMP_FILE_PATH='${SKILLS_DUMP_FILE_PATH}' skills::_parse_yaml_document '${SKILLS_DUMP_FILE_PATH}' 2>&1 " - echo "$output" | grep -qi "line 6" + echo "$output" | grep -qi "line 5" echo "$output" | grep -qi "badfield" }