diff --git a/docs/PLUGIN-PHILOSOPHY.md b/docs/PLUGIN-PHILOSOPHY.md index e4488acbb7..31f59bd518 100644 --- a/docs/PLUGIN-PHILOSOPHY.md +++ b/docs/PLUGIN-PHILOSOPHY.md @@ -261,6 +261,14 @@ For project configuration, use neutral repository-relative paths anchored at when the contract requires containment, and document precedence. Do not add an environment variable merely to create a second configuration channel. +Apply the same anchoring rule to bundled assets: one skill citing another skill's supporting file +writes the full `${CLAUDE_PLUGIN_ROOT}/skills//` form, optionally paired with a +relative markdown link target for browsing on GitHub — for example +``[`${CLAUDE_PLUGIN_ROOT}/skills/audit/context/suppression.md`](../audit/context/suppression.md)``. +A bare `context/…`-style path is reserved for a skill's OWN supporting files; it resolves against +the citing skill's directory, so a cross-skill citation written that way points at a file that is +not there. + ## Setup is explicit and repeatable A plugin requires a `setup` skill iff it has (a) a consumer-project configuration surface, (b) an diff --git a/plugins/skill-quality/.claude-plugin/plugin.json b/plugins/skill-quality/.claude-plugin/plugin.json index 6122d6a936..ec8c1c2510 100644 --- a/plugins/skill-quality/.claude-plugin/plugin.json +++ b/plugins/skill-quality/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "$schema": "https://json.schemastore.org/claude-code-plugin-manifest.json", "name": "skill-quality", - "version": "0.15.0", + "version": "0.15.1", "description": "Skill-authoring QA tooling: a static contract checker that runs twenty-two deterministic checks over a Claude Code skill (frontmatter, per-skill listing-entry cap, trigger-keyword preservation, line caps, broken internal refs, markdownlint, gotchas surface, evals presence, precompute opportunity, injection shell-declaration, fresh-eyes declaration conformance), a shared skill-listing budget reporter across a set of skills, and a bundled evals.json schema plus a deterministic eval-quality lint (duplicate case identities, missing fixtures, empty or vague grading criteria, set-coverage warnings). Runs against any repo's skills directory via the convention-resolution ladder — no baked layout.", "author": { "name": "Melodic Software", diff --git a/plugins/skill-quality/CHANGELOG.md b/plugins/skill-quality/CHANGELOG.md index fdc2c2941b..2a8bf47fa1 100644 --- a/plugins/skill-quality/CHANGELOG.md +++ b/plugins/skill-quality/CHANGELOG.md @@ -3,6 +3,24 @@ All notable changes to the `skill-quality` plugin are documented here. Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); this plugin uses semantic versioning. +## [0.15.1] + +### Fixed + +- **A cross-skill citation reported as a broken internal ref pointed at the wrong directory.** + Check 5 resolves every bare `context/…`-shaped path against the CITING skill's own directory, so + a skill citing a sibling skill's supporting file failed with "no such file under the skill dir" + while the file plainly existed one directory over. The message sent the author looking for the + file where it could never be. When the unresolved path does resolve under a sibling skill of the + same skills root, the finding now also names that sibling and the citation form that works — + `${CLAUDE_PLUGIN_ROOT}/skills//` in a plugin-shaped root, `..//` + outside one, where that variable is undefined. Still a FAIL: the bare form really does resolve + against the citing skill, so it is wrong regardless of where the file lives. The sibling hit is + evidence, not proof — this check deliberately extracts prose and inline-code refs, so a generic + path can collide with an unrelated same-named sibling file — so the original hand-verify wording + is kept and the suggestion is phrased conditionally. A path no sibling hosts is unchanged. + Extraction is unchanged too: prose and inline-code refs are still in scope, deliberately. + ## [0.15.0] ### Changed diff --git a/plugins/skill-quality/README.md b/plugins/skill-quality/README.md index cfd888fc92..714a35e3c1 100644 --- a/plugins/skill-quality/README.md +++ b/plugins/skill-quality/README.md @@ -24,7 +24,10 @@ phrase, which quietly degrades a skill's auto-invocation. Check 3 compares the t truncates that entry) — a different, narrower limit from the shared budget below. - Trigger-keyword preservation vs `HEAD` (skipped for a new, uncommitted skill). - `SKILL.md` under 500 lines (hard) / 200 lines (soft, advisory). -- Backtick- and link-cited skill-internal supporting files resolve. +- Backtick- and link-cited skill-internal supporting files resolve — when a path that misses instead + resolves under a sibling skill, the finding names that sibling and the + `${CLAUDE_PLUGIN_ROOT}/skills//...` cross-skill form, while keeping the hand-verify + caveat (the sibling hit is evidence, not proof: paths can collide). - `markdownlint-cli2` clean (advisory-skips when `npx` is absent). - `scripts/*.test.sh` pass where present. - Vendored `vendor/` byte-identical vs `HEAD`; stale-tracking metadata keys preserved; sync age. diff --git a/plugins/skill-quality/scripts/check-skill.sh b/plugins/skill-quality/scripts/check-skill.sh index f28fca161d..67c37235a9 100755 --- a/plugins/skill-quality/scripts/check-skill.sh +++ b/plugins/skill-quality/scripts/check-skill.sh @@ -40,7 +40,10 @@ # sibling did not carry at the base ref — WARNs, since the marketplace # listing still routes it; lost phrases and coincidental overlap FAIL) # 4. SKILL.md < 500 lines (hard cap) -# 5. Backtick-cited skill-internal supporting files resolve +# 5. Backtick-cited skill-internal supporting files resolve (a path that misses +# here but resolves under a SIBLING skill also names that sibling and the +# `${CLAUDE_PLUGIN_ROOT}/skills//…` cross-skill form, without +# dropping the hand-verify caveat — the hit may be a name collision) # 6. markdownlint clean (markdownlint-cli2; WARN-skip if npx absent) # 7. scripts/*.test.sh pass where present # 8. vendor/ byte-identical vs HEAD, unless paired with an upstream-version @@ -197,6 +200,15 @@ if [[ ! -f "$SKILL_MD" ]]; then exit 1 fi +# Is this skill bundled in a PLUGIN, or a loose skill under some skills root? +# A plugin manifest two levels up (/skills//) is the marker. +# Several checks branch on it — check 1 (a declared `name` also registers a bare +# alias there) and check 5 (a cross-skill citation anchors at +# ${CLAUDE_PLUGIN_ROOT}, which is undefined outside a plugin) — so the layout +# convention is asserted in ONE place rather than restated per call site. +IS_PLUGIN_SKILL=0 +[[ -f "$SKILL_DIR/../../.claude-plugin/plugin.json" ]] && IS_PLUGIN_SKILL=1 + # --- Check 1: frontmatter parses; description present; declared name matches -- FRONTMATTER="$(skill_frontmatter::extract <"$SKILL_MD")" @@ -229,7 +241,7 @@ else err "frontmatter name '$CUR_NAME' is not kebab-case ([a-z0-9] and hyphens, per the Agent Skills spec)" elif [[ -n "$CUR_NAME" && "$CUR_NAME" != "$SKILL_NAME" ]]; then err "frontmatter name '$CUR_NAME' does not match skill directory '$SKILL_NAME'" - elif [[ -n "$CUR_NAME" && -f "$SKILL_DIR/../../.claude-plugin/plugin.json" ]]; then + elif [[ -n "$CUR_NAME" && "$IS_PLUGIN_SKILL" == 1 ]]; then # In a PLUGIN skill a matching `name` is not inert: a declared name also # answers to the bare `/` unless another command owns that token # (https://code.claude.com/docs/en/skills#how-a-skill-gets-its-command-name), @@ -363,7 +375,47 @@ while IFS= read -r ref; do fi if [[ ! -e "$SKILL_DIR/$ref" ]]; then ref_line="$(grep -nF "$ref" "$SKILL_MD" 2>/dev/null | head -1 | cut -d: -f1)" - err "broken skill-internal ref: $ref (no such file under the skill dir; cited at SKILL.md:${ref_line:-?} — hand-verify the line before fixing, may be an illustrative example)" + # A path that misses here but DOES resolve under a SIBLING skill of the same + # skills root is most often a cross-skill citation written in the + # skill-internal form — not a missing file. Every bare path in this check + # resolves against the CITING skill's dir, so the bare form is wrong either + # way and still FAILs; what changes is the message. The default wording + # sends the author looking for the file under their own skill, where it will + # never be — naming the host sibling and the anchored form that works is the + # difference between a dead end and a one-line fix. + # + # The sibling hit is EVIDENCE, not proof: this check deliberately extracts + # prose and inline-code refs, so a generic path (`scripts/run.sh`) can + # collide with an unrelated same-named sibling file. The wording therefore + # stays conditional and keeps the hand-verify instruction the default + # message carries — a coincidental name match and an illustrative example + # are both still live readings. Glob order is sorted, so a path present + # under more than one sibling names the first deterministically. + REF_HOST="" + for other_md in "$SKILLS_ROOT"/*/SKILL.md; do + [[ -f "$other_md" ]] || continue + [[ "$other_md" == "$SKILL_MD" ]] && continue + if [[ -e "${other_md%/SKILL.md}/$ref" ]]; then + REF_HOST="${other_md%/SKILL.md}" + REF_HOST="${REF_HOST##*/}" + break + fi + done + if [[ -z "$REF_HOST" ]]; then + err "broken skill-internal ref: $ref (no such file under the skill dir; cited at SKILL.md:${ref_line:-?} — hand-verify the line before fixing, may be an illustrative example)" + else + # Plugin-shaped root: bundled plugin assets are anchored at the plugin + # root, so that is the form to name. Outside a plugin + # ${CLAUDE_PLUGIN_ROOT} is undefined, so name the layout-free + # sibling-relative form rather than advertise a variable that resolves to + # nothing. + if [[ "$IS_PLUGIN_SKILL" == 1 ]]; then + ref_fix="\${CLAUDE_PLUGIN_ROOT}/skills/$REF_HOST/$ref" + else + ref_fix="../$REF_HOST/$ref" + fi + err "broken skill-internal ref: $ref (no such file under the skill dir; cited at SKILL.md:${ref_line:-?} — hand-verify the line before fixing, may be an illustrative example). A file with that path DOES exist under sibling skill '$REF_HOST': if that is the file meant, this is a cross-skill citation, and a bare path always resolves against the CITING skill's dir — write it as $ref_fix. If the names merely collide, the ref is unrelated to that sibling." + fi fi done < <( { diff --git a/plugins/skill-quality/scripts/check-skill.test.sh b/plugins/skill-quality/scripts/check-skill.test.sh index 8155123ccb..b69117000b 100755 --- a/plugins/skill-quality/scripts/check-skill.test.sh +++ b/plugins/skill-quality/scripts/check-skill.test.sh @@ -2285,6 +2285,117 @@ else fail "redundancy warning should be plugin-scoped" fi +# 36a. Check 5: a bare path that misses under the citing skill but resolves under +# a SIBLING skill is most likely a cross-skill citation, not a missing file. +# It still FAILs (the bare form really does resolve against the citing +# skill), but in a plugin-shaped skills root the message must name the host +# sibling and the `${CLAUDE_PLUGIN_ROOT}` form — the default wording alone +# sends the author hunting under their own skill, where the file will never +# be. The hand-verify caveat stays: the hit is evidence, not proof. +mkdir -p "$PLUGIN_SKILLS/xref-host/context" "$PLUGIN_SKILLS/xref-citer" +printf 'Sibling-owned supporting file.\n' >"$PLUGIN_SKILLS/xref-host/context/suppression.md" +printf '%s' '--- +description: "Hosts a supporting file. Use when: '"'"'checking cross-skill citations'"'"'." +--- + +## Purpose + +Fixture sibling that owns context/suppression.md. + +## Gotchas + +None known. +' >"$PLUGIN_SKILLS/xref-host/SKILL.md" +printf '%s' '--- +description: "Cites a sibling file. Use when: '"'"'checking cross-skill citations'"'"'." +--- + +## Purpose + +Cites the sibling-owned `context/suppression.md` in the skill-internal form. + +## Gotchas + +None known. +' >"$PLUGIN_SKILLS/xref-citer/SKILL.md" +out="$( (cd "$TMP" && + CHECK_SKILL_SKILLS_ROOT="$PLUGIN_SKILLS" CHECK_SKILL_SKIP_MARKDOWNLINT=1 \ + bash "$SUT" xref-citer) 2>&1)" +rc=$? +if [[ $rc -eq 1 ]] && + grep -q 'broken skill-internal ref: context/suppression.md' <<<"$out" && + grep -q "sibling skill 'xref-host'" <<<"$out" && + grep -qF '${CLAUDE_PLUGIN_ROOT}/skills/xref-host/context/suppression.md' <<<"$out" && + grep -q 'hand-verify the line' <<<"$out"; then + pass "a sibling-hosted ref names the sibling and the plugin-root form" +else + fail "sibling-hosted ref should name the sibling and the plugin-root form (rc=$rc): $out" +fi + +# 36b. Same detection outside a plugin: `${CLAUDE_PLUGIN_ROOT}` is undefined +# there, so the message must name the layout-free sibling-relative form +# instead of advertising a variable the consumer does not have. +mkdir -p "$SKILLS/plain-host/reference" +printf 'Sibling-owned supporting file.\n' >"$SKILLS/plain-host/reference/scaling.md" +make_skill plain-host '--- +description: "Hosts a supporting file. Use when: '"'"'checking non-plugin cross-skill citations'"'"'." +--- + +## Purpose + +Fixture sibling that owns reference/scaling.md. + +## Gotchas + +None known. +' +make_skill plain-citer '--- +description: "Cites a sibling file. Use when: '"'"'checking non-plugin cross-skill citations'"'"'." +--- + +## Purpose + +Cites the sibling-owned `reference/scaling.md` in the skill-internal form. + +## Gotchas + +None known. +' +out="$(run plain-citer 2>&1)" +rc=$? +if [[ $rc -eq 1 ]] && + grep -q "sibling skill 'plain-host'" <<<"$out" && + grep -q '\.\./plain-host/reference/scaling\.md' <<<"$out" && + ! grep -q 'skills/plain-host/reference/scaling\.md' <<<"$out"; then + pass "outside a plugin the sibling message names the relative form, not the plugin root" +else + fail "non-plugin sibling message should name ../plain-host/... (rc=$rc): $out" +fi + +# 36c. False-positive guard: a ref no sibling hosts is still a plain broken +# internal ref, with the original hand-verify wording intact. +make_skill orphan-citer '--- +description: "Cites a file nobody owns. Use when: '"'"'checking the broken-ref message'"'"'." +--- + +## Purpose + +Cites `context/nowhere-at-all.md`, which no skill in this root owns. + +## Gotchas + +None known. +' +out="$(run orphan-citer 2>&1)" +rc=$? +if [[ $rc -eq 1 ]] && + grep -q 'broken skill-internal ref: context/nowhere-at-all.md' <<<"$out" && + ! grep -q 'sibling skill' <<<"$out"; then + pass "a ref no sibling hosts keeps the plain broken-internal-ref message" +else + fail "unhosted ref should keep the broken-internal-ref message (rc=$rc): $out" +fi + if [[ $fails -ne 0 ]]; then printf '%d assertion(s) failed\n' "$fails" >&2 exit 1