fix(kyberforge): address self-audit findings and update lessons
- Reorder skill-audit description to lead with 'Use when...' trigger (P3) - Add concrete example to 'control calibration' body discipline check (P4) - Add bats test files to README file tables for both skills - Fix REPO_ROOT and SCRIPT paths in bats files after tests/ subdirectory removed - Add three lessons: plugin cache isolation, spec-grounded rubrics, test file placement Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
12
LESSONS.md
12
LESSONS.md
@@ -98,6 +98,18 @@ When running a full test audit, `claude plugin validate --strict` was not includ
|
||||
|
||||
The pre-commit hook ran `shellcheck "$f"` without `-x`. Without `-x`, shellcheck fires SC1091 for every `source` statement and exits non-zero, blocking the commit. This was a latent bug since the hook was written, only triggered when `install.sh` (which sources `deploy-manifest.sh`) was staged for the first time. Compounding it: the `# shellcheck source=` directive in `install.sh` pointed to `deploy-manifest.sh` (bare filename, resolved from CWD = repo root) rather than `scripts/deploy-manifest.sh` (correct repo-root-relative path), so even with `-x` the file wasn't found on the first attempt. Fix: always pass `-x` to shellcheck in hooks. When writing a `source=` directive, use a path that resolves correctly from the CWD where shellcheck will be invoked — verify with `shellcheck -x <file>` before committing.
|
||||
|
||||
## 2026-06-22 — Plugin cache isolation rules out shared/ directories between skills
|
||||
|
||||
When two skills in the same plugin share a resource (e.g. validate.sh), the instinct is to put it in a shared/ directory and reference it with a relative path. This breaks silently after install: plugins are copied to a cache, and `../` paths across skill directories stop resolving. The correct pattern is duplication with clear ownership — one skill owns the canonical copy and the other delegates to it via a skill invocation (e.g. /skill-audit) rather than a file path. If delegation is not possible, duplicate the file and note the owning skill in a comment.
|
||||
|
||||
## 2026-06-22 — Qualitative rubrics should be grounded in upstream spec docs, not derived from in-repo usage
|
||||
|
||||
When skill-audit's qualitative checks for description quality and body discipline were first written, they were derived from skill-write's own authoring conventions — a circular dependency. Any drift in skill-write's conventions would silently propagate into the audit criteria. Fix: extract condensed reference files directly from the upstream spec (agentskills.io) and load them conditionally from the audit skill. The rubric is then grounded in the authoritative source and independent of in-repo convention drift.
|
||||
|
||||
## 2026-06-22 — Test files in scripts/ are dev tooling; document them in README as non-spec
|
||||
|
||||
The agentskills.io spec defines scripts/ for bundled executable scripts — it says nothing about test infrastructure. Bats test files placed in scripts/ (or scripts/tests/) are invisible to auditors following the spec and create silent README drift if not documented. Fix: place test files directly in scripts/ (no subdirectory), add a row to the README file table for each with a "dev tooling, not shipped with the plugin" note, and don't nest them in a tests/ subdirectory since that creates a non-spec directory structure.
|
||||
|
||||
## 2026-05-18 — Planning meta-commentary does not belong in deployed artifacts
|
||||
|
||||
During write-skill refactor, an "open thread" note (about a deferred research step) was written directly into the SKILL.md Process section. The user caught it. The rule it violated: a deployed artifact (SKILL.md, a runtime file loaded by agents) must not contain planning meta-commentary — deferred items, open threads, and implementation notes belong in the issue file, which is the planning artifact. The skill body should contain only content relevant to runtime execution. If a decision is deferred, record it in the issue and leave no trace in the skill. The distinction: issue = planning record; skill = executable instruction.
|
||||
|
||||
@@ -25,4 +25,5 @@ Provide the path to the skill directory to audit when invoking.
|
||||
| `scripts/validate.sh` | Structural validator — checks name format, name matches directory, description length, line count, placeholder detection, script executable bit, and interactive-prompt detection |
|
||||
| `references/description-quality.md` | Spec-grounded rubric for description auditing — loaded when a finding is borderline |
|
||||
| `references/body-discipline.md` | Spec-grounded rubric for body discipline auditing — loaded when padding vs necessity is unclear |
|
||||
| `scripts/validate.bats` | Bats test suite for validate.sh — dev tooling, not shipped with the plugin |
|
||||
| `README.md` | This file |
|
||||
|
||||
@@ -1,13 +1,13 @@
|
||||
---
|
||||
name: skill-audit
|
||||
description: >
|
||||
Audit a skill directory against the agentskills.io specification — structural
|
||||
Use when the user wants to review a skill they wrote, says "audit this skill",
|
||||
"check if my skill follows best practices", "review my SKILL.md", or wants to
|
||||
know if a skill is ready to ship — even if they don't use the word "audit".
|
||||
Audits a skill directory against the agentskills.io specification — structural
|
||||
checks plus qualitative review of description quality, body discipline, formatting,
|
||||
file structure, and internal consistency. Produces a PASS/FAIL/SUGGESTION punch
|
||||
list with a specific fix proposal for every FAIL. Use when the user wants to
|
||||
review a skill they wrote, says "audit this skill", "check if my skill follows
|
||||
best practices", "review my SKILL.md", or wants to know if a skill is ready to
|
||||
ship — even if they don't use the word "audit". Do not use to run evals, fix
|
||||
list with a specific fix proposal for every FAIL. Do not use to run evals, fix
|
||||
application code bugs, or perform general code review unrelated to skill quality.
|
||||
allowed-tools: Bash Read
|
||||
metadata:
|
||||
@@ -46,7 +46,7 @@ For each sentence in the body, apply: *"Would the agent get this wrong without t
|
||||
|
||||
- **Defaults not menus**: every decision point gives one default + one escape hatch, not a list of options
|
||||
- **Why rationale**: include/exclude rules explain why, not just what
|
||||
- **Control calibration**: prescriptive for fragile or critical sequences; flexible where multiple approaches are valid
|
||||
- **Control calibration**: prescriptive for fragile or critical sequences (e.g. a script invocation where flag order or exact arguments must not change); flexible where multiple approaches are valid
|
||||
|
||||
If uncertain whether a sentence is padding or whether a control decision is correctly calibrated, read `references/body-discipline.md`.
|
||||
|
||||
|
||||
198
plugins/kyberforge/skills/skill-audit/scripts/validate.bats
Normal file
198
plugins/kyberforge/skills/skill-audit/scripts/validate.bats
Normal file
@@ -0,0 +1,198 @@
|
||||
#!/usr/bin/env bats
|
||||
|
||||
setup() {
|
||||
REPO_ROOT="$(cd "$BATS_TEST_DIRNAME/../../../../../" && pwd)"
|
||||
load "$REPO_ROOT/tests/test_helper/bats-support/load"
|
||||
load "$REPO_ROOT/tests/test_helper/bats-assert/load"
|
||||
|
||||
SCRIPT="$(cd "$(dirname "$BATS_TEST_FILENAME")" && pwd)/validate.sh"
|
||||
TMPDIR="$(mktemp -d)"
|
||||
|
||||
# Helper: create a minimal valid skill directory
|
||||
make_valid_skill() {
|
||||
local dir="$1"
|
||||
local name
|
||||
name="$(basename "$dir")"
|
||||
mkdir -p "$dir/scripts"
|
||||
cat > "$dir/SKILL.md" <<EOF
|
||||
---
|
||||
name: $name
|
||||
description: A valid skill description that is well within the limit.
|
||||
---
|
||||
|
||||
## Step 1
|
||||
|
||||
Do the thing.
|
||||
EOF
|
||||
}
|
||||
}
|
||||
|
||||
teardown() {
|
||||
rm -rf "$TMPDIR"
|
||||
}
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Passing cases
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@test "passes on a valid minimal skill" {
|
||||
local skill="$TMPDIR/my-skill"
|
||||
make_valid_skill "$skill"
|
||||
run bash "$SCRIPT" "$skill"
|
||||
assert_success
|
||||
}
|
||||
|
||||
@test "--help exits 0" {
|
||||
run bash "$SCRIPT" --help
|
||||
assert_success
|
||||
assert_output --partial "Usage:"
|
||||
}
|
||||
|
||||
@test "passes when scripts/ directory is absent" {
|
||||
local skill="$TMPDIR/my-skill"
|
||||
make_valid_skill "$skill"
|
||||
rmdir "$skill/scripts"
|
||||
run bash "$SCRIPT" "$skill"
|
||||
assert_success
|
||||
}
|
||||
|
||||
@test "FILL IN: inside backticks does not fail" {
|
||||
local skill="$TMPDIR/my-skill"
|
||||
make_valid_skill "$skill"
|
||||
echo "Use \`FILL IN: value\` as an example." >> "$skill/SKILL.md"
|
||||
run bash "$SCRIPT" "$skill"
|
||||
assert_success
|
||||
}
|
||||
|
||||
@test "passes at exactly 1024-char description" {
|
||||
local skill="$TMPDIR/my-skill"
|
||||
local name
|
||||
name="$(basename "$skill")"
|
||||
mkdir -p "$skill"
|
||||
local desc
|
||||
desc="$(python3 -c "print('x' * 1024)")"
|
||||
cat > "$skill/SKILL.md" <<EOF
|
||||
---
|
||||
name: $name
|
||||
description: $desc
|
||||
---
|
||||
|
||||
## Step 1
|
||||
|
||||
Do the thing.
|
||||
EOF
|
||||
run bash "$SCRIPT" "$skill"
|
||||
assert_success
|
||||
}
|
||||
|
||||
@test "passes at exactly 500 lines" {
|
||||
local skill="$TMPDIR/my-skill"
|
||||
make_valid_skill "$skill"
|
||||
local current
|
||||
current="$(wc -l < "$skill/SKILL.md")"
|
||||
local needed=$(( 500 - current ))
|
||||
python3 -c "print('\n' * $needed, end='')" >> "$skill/SKILL.md"
|
||||
run bash "$SCRIPT" "$skill"
|
||||
assert_success
|
||||
}
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Failing cases
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@test "fails when SKILL.md is missing" {
|
||||
local skill="$TMPDIR/my-skill"
|
||||
mkdir -p "$skill"
|
||||
run bash "$SCRIPT" "$skill"
|
||||
assert_failure
|
||||
}
|
||||
|
||||
@test "fails when name does not match directory" {
|
||||
local skill="$TMPDIR/my-skill"
|
||||
make_valid_skill "$skill"
|
||||
sed -i 's/^name: .*/name: wrong-name/' "$skill/SKILL.md"
|
||||
run bash "$SCRIPT" "$skill"
|
||||
assert_failure
|
||||
}
|
||||
|
||||
@test "fails when description exceeds 1024 chars" {
|
||||
local skill="$TMPDIR/my-skill"
|
||||
local name
|
||||
name="$(basename "$skill")"
|
||||
mkdir -p "$skill"
|
||||
local desc
|
||||
desc="$(python3 -c "print('x' * 1025)")"
|
||||
cat > "$skill/SKILL.md" <<EOF
|
||||
---
|
||||
name: $name
|
||||
description: $desc
|
||||
---
|
||||
|
||||
## Step 1
|
||||
|
||||
Do the thing.
|
||||
EOF
|
||||
run bash "$SCRIPT" "$skill"
|
||||
assert_failure
|
||||
}
|
||||
|
||||
@test "fails when SKILL.md exceeds 500 lines" {
|
||||
local skill="$TMPDIR/my-skill"
|
||||
make_valid_skill "$skill"
|
||||
python3 -c "print('\n' * 500)" >> "$skill/SKILL.md"
|
||||
run bash "$SCRIPT" "$skill"
|
||||
assert_failure
|
||||
}
|
||||
|
||||
@test "fails when body contains unfilled FILL IN: placeholder" {
|
||||
local skill="$TMPDIR/my-skill"
|
||||
make_valid_skill "$skill"
|
||||
echo "FILL IN: replace this" >> "$skill/SKILL.md"
|
||||
run bash "$SCRIPT" "$skill"
|
||||
assert_failure
|
||||
}
|
||||
|
||||
@test "fails when a script is not executable" {
|
||||
local skill="$TMPDIR/my-skill"
|
||||
make_valid_skill "$skill"
|
||||
echo "#!/usr/bin/env bash" > "$skill/scripts/helper.sh"
|
||||
chmod -x "$skill/scripts/helper.sh"
|
||||
run bash "$SCRIPT" "$skill"
|
||||
assert_failure
|
||||
}
|
||||
|
||||
@test "fails when a script has an interactive prompt" {
|
||||
local skill="$TMPDIR/my-skill"
|
||||
make_valid_skill "$skill"
|
||||
printf '#!/usr/bin/env bash\nread -p "Enter value: " VAL\n' > "$skill/scripts/helper.sh"
|
||||
chmod +x "$skill/scripts/helper.sh"
|
||||
run bash "$SCRIPT" "$skill"
|
||||
assert_failure
|
||||
}
|
||||
|
||||
@test "fails when name contains consecutive hyphens" {
|
||||
local skill="$TMPDIR/my--skill"
|
||||
make_valid_skill "$skill"
|
||||
run bash "$SCRIPT" "$skill"
|
||||
assert_failure
|
||||
}
|
||||
|
||||
@test "fails when name has a leading hyphen" {
|
||||
local skill="$TMPDIR/-my-skill"
|
||||
make_valid_skill "$skill"
|
||||
run bash "$SCRIPT" "$skill"
|
||||
assert_failure
|
||||
}
|
||||
|
||||
@test "fails when no frontmatter block is present" {
|
||||
local skill="$TMPDIR/my-skill"
|
||||
mkdir -p "$skill"
|
||||
echo "Just some content with no frontmatter." > "$skill/SKILL.md"
|
||||
run bash "$SCRIPT" "$skill"
|
||||
assert_failure
|
||||
}
|
||||
|
||||
@test "fails when no arguments are given" {
|
||||
run bash "$SCRIPT"
|
||||
assert_failure
|
||||
}
|
||||
@@ -35,6 +35,7 @@ This skill produces its best output when you arrive with rich context:
|
||||
| `assets/templates/scripts/README.md` | Placeholder for bundled scripts |
|
||||
| `assets/templates/references/README.md` | Placeholder for reference docs |
|
||||
| `assets/templates/assets/README.md` | Placeholder for static assets |
|
||||
| `scripts/new-skill.bats` | Bats test suite for new-skill.sh — dev tooling, not shipped with the plugin |
|
||||
|
||||
## Placement
|
||||
|
||||
|
||||
111
plugins/kyberforge/skills/skill-write/scripts/new-skill.bats
Normal file
111
plugins/kyberforge/skills/skill-write/scripts/new-skill.bats
Normal file
@@ -0,0 +1,111 @@
|
||||
#!/usr/bin/env bats
|
||||
|
||||
setup() {
|
||||
REPO_ROOT="$(cd "$BATS_TEST_DIRNAME/../../../../../" && pwd)"
|
||||
load "$REPO_ROOT/tests/test_helper/bats-support/load"
|
||||
load "$REPO_ROOT/tests/test_helper/bats-assert/load"
|
||||
|
||||
SCRIPT="$(cd "$(dirname "$BATS_TEST_FILENAME")" && pwd)/new-skill.sh"
|
||||
DEST="$(mktemp -d)"
|
||||
}
|
||||
|
||||
teardown() {
|
||||
rm -rf "$DEST"
|
||||
}
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Passing cases
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@test "--help exits 0" {
|
||||
run bash "$SCRIPT" --help
|
||||
assert_success
|
||||
assert_output --partial "Usage:"
|
||||
}
|
||||
|
||||
@test "creates scaffold directory at destination" {
|
||||
run bash "$SCRIPT" my-tool "$DEST"
|
||||
assert_success
|
||||
assert [ -d "$DEST/my-tool" ]
|
||||
}
|
||||
|
||||
@test "scaffold contains SKILL.md" {
|
||||
bash "$SCRIPT" my-tool "$DEST"
|
||||
assert [ -f "$DEST/my-tool/SKILL.md" ]
|
||||
}
|
||||
|
||||
@test "scaffold contains README.md" {
|
||||
bash "$SCRIPT" my-tool "$DEST"
|
||||
assert [ -f "$DEST/my-tool/README.md" ]
|
||||
}
|
||||
|
||||
@test "scaffold contains scripts/, references/, assets/ directories" {
|
||||
bash "$SCRIPT" my-tool "$DEST"
|
||||
assert [ -d "$DEST/my-tool/scripts" ]
|
||||
assert [ -d "$DEST/my-tool/references" ]
|
||||
assert [ -d "$DEST/my-tool/assets" ]
|
||||
}
|
||||
|
||||
@test "substitutes skill name in SKILL.md" {
|
||||
bash "$SCRIPT" my-tool "$DEST"
|
||||
run grep "my-tool" "$DEST/my-tool/SKILL.md"
|
||||
assert_success
|
||||
}
|
||||
|
||||
@test "substitutes skill name in README.md" {
|
||||
bash "$SCRIPT" my-tool "$DEST"
|
||||
run grep "my-tool" "$DEST/my-tool/README.md"
|
||||
assert_success
|
||||
}
|
||||
|
||||
@test "skill name with numbers is valid" {
|
||||
run bash "$SCRIPT" my-tool-2 "$DEST"
|
||||
assert_success
|
||||
assert [ -d "$DEST/my-tool-2" ]
|
||||
}
|
||||
|
||||
@test "next-steps output references /skill-audit not validate.sh" {
|
||||
run bash "$SCRIPT" my-tool "$DEST"
|
||||
assert_output --partial "/skill-audit"
|
||||
refute_output --partial "validate.sh"
|
||||
}
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Failing cases
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@test "fails when no arguments given" {
|
||||
run bash "$SCRIPT"
|
||||
assert_failure
|
||||
}
|
||||
|
||||
@test "fails when skill name contains uppercase" {
|
||||
run bash "$SCRIPT" MyTool "$DEST"
|
||||
assert_failure
|
||||
}
|
||||
|
||||
@test "fails when skill name has consecutive hyphens" {
|
||||
run bash "$SCRIPT" my--tool "$DEST"
|
||||
assert_failure
|
||||
}
|
||||
|
||||
@test "fails when skill name has a leading hyphen" {
|
||||
run bash "$SCRIPT" -my-tool "$DEST"
|
||||
assert_failure
|
||||
}
|
||||
|
||||
@test "fails when skill name has a trailing hyphen" {
|
||||
run bash "$SCRIPT" my-tool- "$DEST"
|
||||
assert_failure
|
||||
}
|
||||
|
||||
@test "fails when destination directory does not exist" {
|
||||
run bash "$SCRIPT" my-tool "/nonexistent/path"
|
||||
assert_failure
|
||||
}
|
||||
|
||||
@test "fails when target already exists" {
|
||||
mkdir -p "$DEST/my-tool"
|
||||
run bash "$SCRIPT" my-tool "$DEST"
|
||||
assert_failure
|
||||
}
|
||||
Reference in New Issue
Block a user