diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index dcd906cdb..6134ae965 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -282,6 +282,20 @@ jobs: - name: Run cross-plugin-source-drift tests run: bash scripts/check-cross-plugin-source-drift.test.sh + skill-leaf-name-gate: + runs-on: ubuntu-24.04 + timeout-minutes: 15 + steps: + - name: Check out + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + persist-credentials: false + # Self-test first, so a broken detector cannot mask a regression. + - name: Run skill-leaf-name tests + run: bash scripts/check-skill-leaf-names.test.sh + - name: Check for unregistered cross-plugin skill leaf-name collisions + run: scripts/check-skill-leaf-names.sh --check + silent-skip-gate: runs-on: ubuntu-24.04 timeout-minutes: 15 @@ -577,6 +591,7 @@ jobs: - hook-utils-sync - standards-contract-sync - cross-plugin-source-drift + - skill-leaf-name-gate - silent-skip-gate - orphaned-fixture-gate - changelog-parity-gate diff --git a/scripts/check-skill-leaf-names.sh b/scripts/check-skill-leaf-names.sh new file mode 100755 index 000000000..72847a901 --- /dev/null +++ b/scripts/check-skill-leaf-names.sh @@ -0,0 +1,193 @@ +#!/usr/bin/env bash +# Discover and check cross-plugin skill leaf-name collisions: a skill directory +# name (the leaf of `/:`) carried by 2+ plugins. +# +# scripts/check-skill-leaf-names.sh discover: list every leaf name +# owned by 2+ plugins, with its +# owners and registration state +# scripts/check-skill-leaf-names.sh --check fail on an UNREGISTERED +# collision, on a registered one +# whose OWNER SET changed, or on +# an entry that no longer collides +# +# Namespacing already guarantees every one of these is separately invocable -- +# `/disk-hygiene:clean` and `/repo-hygiene:clean` can never resolve to each +# other, and the philosophy's Naming section is explicit that a built-in never +# forces a plugin skill's name. So a collision is not a correctness bug and this +# script is not a rename mandate. +# +# What it catches is the thing namespacing does NOT cover: the slash-command +# picker labels a row by the LEAF name and keeps `:` as a hidden +# alias, so two colliding skills read identically in the listing and are told +# apart only by the `()` prefix the description carries. That cost +# is invisible from inside any one plugin -- nothing in a single skill's own +# review surfaces it -- and the grammar's own collision rule +# (docs/PLUGIN-PHILOSOPHY.md, Naming) governs siblings WITHIN one namespace, not +# across plugins. +# +# Registering a leaf name in skill-leaf-name-registry.txt records the grounds it +# was accepted on, together with the owner set those grounds were argued over. +# A new plugin joining an already-registered collision has to be argued on its +# own merits, so the owner set is part of the entry rather than the bare name -- +# otherwise the first registration would silently pre-authorize every later one. +# An unregistered collision is a decision waiting to be made, not yet a +# violation of anything. +set -euo pipefail + +cd "$(dirname "${BASH_SOURCE[0]}")/.." + +registry="scripts/skill-leaf-name-registry.txt" + +# leaf name -> space-separated owning plugin names +declare -A leaf_owners + +for plugin_dir in plugins/*/; do + plugin="${plugin_dir%/}" + plugin="${plugin##*/}" + skills_dir="${plugin_dir}skills" + [[ -d "$skills_dir" ]] || continue + for skill_dir in "$skills_dir"/*/; do + [[ -f "${skill_dir}SKILL.md" ]] || continue + leaf="${skill_dir%/}" + leaf="${leaf##*/}" + leaf_owners["$leaf"]+="${plugin} " + done +done + +# Collisions only: a leaf name owned by 2+ plugins. +# +# `collision_leaves` and `collision_count` shadow the associative array on +# purpose: under `set -u` an associative array that was declared but never +# assigned is UNBOUND, so `${#collisions[@]}` and `${!collisions[@]}` abort the +# script in the zero-collision case -- which is exactly the state the stale-entry +# guard is meant to shepherd the repo into. +declare -A collisions +collision_leaves=() +collision_count=0 +for leaf in "${!leaf_owners[@]}"; do + # shellcheck disable=SC2206 # plugin names are kebab-case; word-splitting is the intent + owners=(${leaf_owners[$leaf]}) + ((${#owners[@]} >= 2)) || continue + collisions["$leaf"]="${leaf_owners[$leaf]}" + collision_leaves+=("$leaf") + collision_count=$((collision_count + 1)) +done + +# leaf name -> accepted owner set: a sorted comma-separated plugin list, or `*` +# for a name whose owner set is open by contract (see the registry header). +declare -A registered +registered_leaves=() +if [[ -f "$registry" ]]; then + while IFS= read -r line; do + line="${line%%#*}" + line="${line#"${line%%[![:space:]]*}"}" + line="${line%"${line##*[![:space:]]}"}" + [[ -z "$line" ]] && continue + # `read`, not array splitting: the owner field may be a literal `*`, which + # unquoted word splitting would pathname-expand against the cwd. + read -r leaf_key owner_field _ <<<"$line" + [[ -n "${registered[$leaf_key]+set}" ]] || registered_leaves+=("$leaf_key") + registered["$leaf_key"]="${owner_field:-}" + done <"$registry" +fi + +# Sorted comma-separated owner set, so the comparison is order-independent. +owner_set() { + printf '%s\n' "$@" | sort | paste -sd, - +} + +mode="${1:-discover}" +case "$mode" in +discover | --check) ;; +*) + echo "usage: $(basename "$0") [--check]" >&2 + exit 2 + ;; +esac + +if [[ "$mode" == "discover" ]]; then + if ((collision_count == 0)); then + echo "No cross-plugin skill leaf-name collisions." + exit 0 + fi + for leaf in $(printf '%s\n' ${collision_leaves[@]+"${collision_leaves[@]}"} | sort); do + # shellcheck disable=SC2206 + owners=(${collisions[$leaf]}) + # shellcheck disable=SC2310 # owner_set only sorts strings; nothing inside can fail + actual="$(owner_set "${owners[@]}")" + state="UNREGISTERED" + if [[ -n "${registered[$leaf]+set}" ]]; then + accepted="${registered[$leaf]}" + state="OWNERS-CHANGED" + if [[ "$accepted" == "*" ]]; then + state="registered(*)" + elif [[ -n "$accepted" ]]; then + IFS=',' read -ra accepted_owners <<<"$accepted" + # shellcheck disable=SC2310 # owner_set only sorts strings; nothing inside can fail + accepted="$(owner_set "${accepted_owners[@]}")" + [[ "$accepted" == "$actual" ]] && state="registered" + fi + fi + printf '%-14s %-14s %d plugins: %s\n' \ + "$leaf" "$state" "${#owners[@]}" "$(printf '%s ' "${owners[@]}" | sed 's/ $//')" + done + exit 0 +fi + +failed=0 + +for leaf in $(printf '%s\n' ${collision_leaves[@]+"${collision_leaves[@]}"} | sort); do + # shellcheck disable=SC2206 + owners=(${collisions[$leaf]}) + # shellcheck disable=SC2310 # owner_set only sorts strings; nothing inside can fail + actual="$(owner_set "${owners[@]}")" + + if [[ -z "${registered[$leaf]+set}" ]]; then + printf 'FAIL: skill leaf name %s is now carried by %d plugins (%s) and is not registered.\n' \ + "$leaf" "${#owners[@]}" "${actual//,/ }" >&2 + printf ' These are separately invocable, but the picker labels every row %s.\n' "$leaf" >&2 + printf ' Rename one, or add "%s %s" to %s with the grounds it is accepted on.\n' \ + "$leaf" "$actual" "$registry" >&2 + failed=1 + continue + fi + + accepted="${registered[$leaf]}" + # An open owner set is accepted by contract; only the name is registered. + [[ "$accepted" == "*" ]] && continue + + # Normalize the registry side too, so the comparison is genuinely set-vs-set + # and a hand-edited entry does not fail on ordering alone. + if [[ -n "$accepted" ]]; then + IFS=',' read -ra accepted_owners <<<"$accepted" + # shellcheck disable=SC2310 # owner_set only sorts strings; nothing inside can fail + accepted="$(owner_set "${accepted_owners[@]}")" + fi + + if [[ -z "$accepted" ]]; then + printf 'FAIL: %s registers %s without an owner set. Record it as "%s %s".\n' \ + "$registry" "$leaf" "$leaf" "$actual" >&2 + failed=1 + elif [[ "$accepted" != "$actual" ]]; then + printf 'FAIL: skill leaf name %s is registered for %s but is now carried by %s.\n' \ + "$leaf" "${accepted//,/ }" "${actual//,/ }" >&2 + printf ' A new owner joins an accepted collision on its own grounds, not the old ones.\n' >&2 + printf ' Update the entry to "%s %s" and revisit the rationale above it.\n' "$leaf" "$actual" >&2 + failed=1 + fi +done + +# Stale guard: a registry entry that no longer collides has outlived its reason +# and would otherwise silently pre-authorize a future collision on that name. +for leaf in $(printf '%s\n' ${registered_leaves[@]+"${registered_leaves[@]}"} | sort); do + [[ -n "${collisions[$leaf]:-}" ]] && continue + printf 'FAIL: %s lists %s, but it is no longer carried by 2+ plugins. Drop the entry.\n' \ + "$registry" "$leaf" >&2 + failed=1 +done + +if ((failed)); then + exit 1 +fi + +printf 'All %d cross-plugin skill leaf-name collisions are registered.\n' "$collision_count" diff --git a/scripts/check-skill-leaf-names.test.sh b/scripts/check-skill-leaf-names.test.sh new file mode 100755 index 000000000..3d4f5bbf8 --- /dev/null +++ b/scripts/check-skill-leaf-names.test.sh @@ -0,0 +1,193 @@ +#!/usr/bin/env bash +# Black-box contract test for check-skill-leaf-names.sh. +# +# Self-contained and cwd-independent: builds a throwaway plugins/ tree with +# fixture skills, runs the checker against it, and asserts on exit code + +# output. Mutates only its own mktemp dir. The SUT resolves its paths relative +# to its own location, so the fixture tree carries a copy of it. +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +SUT_SRC="$SCRIPT_DIR/check-skill-leaf-names.sh" + +fails=0 +pass() { printf 'ok - %s\n' "$1"; } +fail() { + printf 'FAIL - %s\n' "$1" >&2 + fails=$((fails + 1)) +} + +TMP="$(mktemp -d)" +trap 'rm -rf "$TMP"' EXIT + +mkdir -p "$TMP/scripts" +cp "$SUT_SRC" "$TMP/scripts/check-skill-leaf-names.sh" +SUT="$TMP/scripts/check-skill-leaf-names.sh" +REGISTRY="$TMP/scripts/skill-leaf-name-registry.txt" + +make_skill() { + # plugin, skill + mkdir -p "$TMP/plugins/$1/skills/$2" + printf -- '---\nname: %s\ndescription: "fixture"\n---\n' "$2" >"$TMP/plugins/$1/skills/$2/SKILL.md" +} + +run() { bash "$SUT" "$@" 2>&1; } + +# A directory without SKILL.md must not count as a skill. +mkdir -p "$TMP/plugins/alpha/skills/not-a-skill" + +make_skill alpha solo +make_skill alpha shared +make_skill beta shared +make_skill gamma shared + +# 1. discover lists a collision with all its owners. +out="$(run)" +if grep -q 'shared' <<<"$out" && grep -q '3 plugins' <<<"$out"; then + pass "discover lists a collision with its owner count" +else + fail "discover should list shared across 3 plugins: $out" +fi + +# 2. A leaf name owned by only one plugin is not a collision. +if ! grep -q 'solo' <<<"$out"; then + pass "single-owner leaf name is not reported" +else + fail "solo should not be reported as a collision: $out" +fi + +# 3. A directory without SKILL.md is not counted as a skill. +if ! grep -q 'not-a-skill' <<<"$out"; then + pass "directory without SKILL.md is not counted" +else + fail "not-a-skill should be ignored: $out" +fi + +# 4. --check fails on an unregistered collision. +: >"$REGISTRY" +out="$(run --check)" +rc=$? +if [[ $rc -eq 1 ]] && grep -q 'is not registered' <<<"$out"; then + pass "unregistered collision fails --check" +else + fail "unregistered collision should fail (rc=$rc): $out" +fi + +# 5. --check passes once the collision is registered with its owner set, and +# comments/blank lines in the registry are ignored. +printf '# a comment\n\n shared alpha,beta,gamma # trailing comment\n' >"$REGISTRY" +out="$(run --check)" +rc=$? +if [[ $rc -eq 0 ]] && grep -q 'are registered' <<<"$out"; then + pass "registered collision passes --check (comments and whitespace ignored)" +else + fail "registered collision should pass (rc=$rc): $out" +fi + +# 5b. Owner order in the registry does not matter — sets are compared, not lists. +printf 'shared gamma,alpha,beta\n' >"$REGISTRY" +out="$(run --check)" +rc=$? +if [[ $rc -eq 0 ]]; then + pass "owner set comparison is order-independent" +else + fail "reordered owner set should pass (rc=$rc): $out" +fi + +# 5c. A registered name whose owner set GREW fails — the first registration must +# not pre-authorize later owners joining on the original grounds. +printf 'shared alpha,beta\n' >"$REGISTRY" +out="$(run --check)" +rc=$? +if [[ $rc -eq 1 ]] && grep -q 'is registered for' <<<"$out"; then + pass "a new owner joining a registered collision fails" +else + fail "grown owner set should fail (rc=$rc): $out" +fi + +# 5d. A bare name with no owner field is rejected, so the old format cannot +# silently keep passing as an open registration. +printf 'shared\n' >"$REGISTRY" +out="$(run --check)" +rc=$? +if [[ $rc -eq 1 ]] && grep -q 'without an owner set' <<<"$out"; then + pass "registration without an owner set is rejected" +else + fail "bare-name entry should fail (rc=$rc): $out" +fi + +# 5e. `*` accepts any owner set — and must reach the comparison as a literal +# rather than being pathname-expanded against the cwd. +printf 'shared *\n' >"$REGISTRY" +out="$(run --check)" +rc=$? +if [[ $rc -eq 0 ]] && grep -q 'are registered' <<<"$out"; then + pass "wildcard owner set accepts any owners (literal, not glob-expanded)" +else + fail "wildcard entry should pass (rc=$rc): $out" +fi + +# 6. Stale guard: a registry entry that no longer collides fails. +printf 'shared\nvanished\n' >"$REGISTRY" +out="$(run --check)" +rc=$? +if [[ $rc -eq 1 ]] && grep -q 'no longer carried by 2+ plugins' <<<"$out"; then + pass "stale registry entry fails --check" +else + fail "stale entry should fail (rc=$rc): $out" +fi + +# 7. A NEW collision on an unregistered name fails even when others are +# registered — the regression-critical path this gate exists for. +printf 'shared\n' >"$REGISTRY" +make_skill alpha newdupe +make_skill beta newdupe +out="$(run --check)" +rc=$? +if [[ $rc -eq 1 ]] && grep -q 'newdupe' <<<"$out"; then + pass "a newly introduced collision fails while existing ones stay registered" +else + fail "new collision should fail (rc=$rc): $out" +fi + +# 7b. The zero-collision end state succeeds. Under `set -u` an associative array +# declared but never assigned is unbound, so a bare ${#a[@]}/${!a[@]} aborts +# here — and this is precisely the state the stale-entry guard exists to +# shepherd the repo into, so it must not be the one state that crashes. +CLEAN="$(mktemp -d)" +mkdir -p "$CLEAN/scripts" "$CLEAN/plugins/solo/skills/only" +cp "$SUT_SRC" "$CLEAN/scripts/check-skill-leaf-names.sh" +printf -- '---\nname: only\ndescription: "fixture"\n---\n' >"$CLEAN/plugins/solo/skills/only/SKILL.md" +: >"$CLEAN/scripts/skill-leaf-name-registry.txt" + +out="$(bash "$CLEAN/scripts/check-skill-leaf-names.sh" --check 2>&1)" +rc=$? +if [[ $rc -eq 0 ]] && ! grep -q 'unbound variable' <<<"$out"; then + pass "zero collisions with an empty registry passes --check" +else + fail "zero-collision state should pass (rc=$rc): $out" +fi + +out="$(bash "$CLEAN/scripts/check-skill-leaf-names.sh" 2>&1)" +rc=$? +if [[ $rc -eq 0 ]] && grep -q 'No cross-plugin skill leaf-name collisions' <<<"$out"; then + pass "zero collisions reports cleanly in discover mode" +else + fail "zero-collision discover should report cleanly (rc=$rc): $out" +fi +rm -rf "$CLEAN" + +# 8. Unknown mode is a usage error, not a silent pass. +bash "$SUT" --bogus >/dev/null 2>&1 +rc=$? +if [[ $rc -eq 2 ]]; then + pass "unknown mode exits 2" +else + fail "unknown mode should exit 2 (rc=$rc)" +fi + +if [[ $fails -ne 0 ]]; then + printf '%d assertion(s) failed\n' "$fails" >&2 + exit 1 +fi +printf 'all assertions passed\n' diff --git a/scripts/skill-leaf-name-registry.txt b/scripts/skill-leaf-name-registry.txt new file mode 100644 index 000000000..756f2901f --- /dev/null +++ b/scripts/skill-leaf-name-registry.txt @@ -0,0 +1,65 @@ +# Skill leaf names deliberately carried by more than one plugin. Read by +# check-skill-leaf-names.sh --check. +# +# Format, one entry per line: +# +# ,,... accepted for exactly this owner set +# * owner set is open by contract +# +# The owner set is part of the entry, not decoration. A new plugin joining an +# already-accepted collision changes the collision being accepted, and has to be +# argued on its own merits -- registering the bare name would silently +# pre-authorize every later owner. Owners are sorted; the check compares sets, +# not order. `*` is reserved for a name whose owner set is fixed by a contract +# that already decided the question, where re-arguing each new owner would be +# ceremony with no decision in it. +# +# --check fails three ways: an unregistered collision, a registered one whose +# owner set has changed, and an entry that no longer collides at all (which has +# outlived its reason and would otherwise pre-authorize a future name). +# +# Every one of these is separately invocable -- namespacing guarantees that, and +# the philosophy's Naming section is explicit that a name is never degraded to +# dodge a collision. What a shared leaf name costs is legibility: the picker +# labels the row by this name, so the entries read identically and are told +# apart only by the `()` prefix on the description. That is the +# cost each entry below is accepted at, and the reason each description's first +# clause has to name its object. + +# Contract-mandated, open owner set. docs/PLUGIN-PHILOSOPHY.md (Setup) fixes the +# name for every plugin that ships consumer configuration; uniformity IS the +# contract, so a new plugin adopting it is conformance, not a new decision. Not +# consolidatable: ${CLAUDE_PLUGIN_ROOT} resolves per containing plugin and there +# is no cross-plugin skill inheritance, so each plugin owns its own. +setup * + +# Fixed verb meaning: read-only findings report (PLUGIN-PHILOSOPHY.md, Naming +# verb table). The namespace supplies the object in every case. Qualifying the +# verb per plugin would say the same thing twice; a topic qualifier is reserved +# for siblings WITHIN one namespace (audit-noise beside audit-encapsulation). +audit claude-config,claude-memory,codebase-health,machine-health,mcp-tools,repo-fleet-hygiene + +# Fixed verb meaning: deterministic pass/fail gate. skill-quality checks a +# skill, toolchain checks a build. +check skill-quality,toolchain + +# Fixed verb meaning: mutates the target. disk-hygiene cleans an arbitrary +# directory tree, repo-hygiene cleans a repository's caches and git metadata. +clean disk-hygiene,repo-hygiene + +# D19, kept on merit. songwriting diagnoses a song, testing diagnoses a failing +# suite -- unrelated domains, and the namespace disambiguates. +diagnose songwriting,testing + +# D19, kept on merit. planning:plan is the root-echo core action of its domain; +# testing:plan is a test-strategy plan. The bare token still belongs to the +# built-in plan-mode toggle, which neither shadows. +plan planning,testing + +# D19, kept on merit. session-flow:workflow routes a development session, +# songwriting:workflow routes a writing session. +workflow session-flow,songwriting + +# Fixed verb meaning: produces an artifact. bug-report writes a report, testing +# writes tests. +write bug-report,testing