feat(ci): register cross-plugin skill leaf-name collisions

[kyle-sexton]

Closes #720

Summary

Eight skill leaf names are carried by more than one plugin. Every one is separately invocable — namespacing guarantees that — so this is not a rename mandate and nothing here changes a single skill name. It makes each collision explicit, dated, and argued, so the next one is a conscious call rather than a discovery.

Fix

scripts/check-skill-leaf-names.sh — mirrors the check-cross-plugin-source-drift.sh idiom exactly, since the problem shape is the same (a repo-wide pattern no single plugin's review can see):

• discover (no args) — lists every leaf name owned by 2+ plugins, its owners, and whether it is registered.
• --check — fails on an unregistered collision, and on a registry entry that no longer collides. The stale guard is the load-bearing half: without it, a leftover entry silently pre-authorizes a future collision on that name.

scripts/skill-leaf-name-registry.txt — seeded with all eight, each carrying the grounds it was accepted on rather than a bare name. setup is contract-mandated by PLUGIN-PHILOSOPHY.md (and not consolidatable — ${CLAUDE_PLUGIN_ROOT} resolves per containing plugin, with no cross-plugin skill inheritance); audit/check/clean/write follow the fixed verb table; diagnose/plan/workflow are D19 keeps.

.github/workflows/ci.yml — new skill-leaf-name-gate lane, added to the ci-status needs: list so the aggregate picks it up. Self-test runs before the check, per the established idiom, so a broken detector cannot mask a regression.

Verification

Discover, against the real tree:

$ scripts/check-skill-leaf-names.sh
audit          registered     6 plugins: claude-config claude-memory codebase-health machine-health mcp-tools repo-fleet-hygiene
check          registered     2 plugins: skill-quality toolchain
clean          registered     2 plugins: disk-hygiene repo-hygiene
diagnose       registered     2 plugins: songwriting testing
plan           registered     2 plugins: planning testing
setup          registered    33 plugins: actionlint ai-briefing autonomy ...
workflow       registered     2 plugins: session-flow songwriting
write          registered     2 plugins: bug-report testing

$ scripts/check-skill-leaf-names.sh --check
All 8 cross-plugin skill leaf-name collisions are registered.

Green on today's tree, so the lane lands without turning unrelated PRs red.

Self-test — 8/8, covering both failure directions and the false-positive direction:

$ bash scripts/check-skill-leaf-names.test.sh
ok   - discover lists a collision with its owner count
ok   - single-owner leaf name is not reported
ok   - directory without SKILL.md is not counted
ok   - unregistered collision fails --check
ok   - registered collision passes --check (comments and whitespace ignored)
ok   - stale registry entry fails --check
ok   - a newly introduced collision fails while existing ones stay registered
ok   - unknown mode exits 2
all assertions passed

shellcheck on both scripts — clean. actionlint on the modified workflow — clean. Both scripts committed 100755, matching the existing scripts/check-*.sh convention (the lane invokes the checker directly, not via bash).

Related

• Refs docs(naming): namespacing guarantees invocation uniqueness, not picker display uniqueness #710 — the picker labels rows by leaf name, which is what makes a shared leaf name cost legibility.
• Refs program: shadowed-skill-renames contract (naming grammar rollout) #280 — D19 "all shadow-dodge names kept on merit"; this registers those keeps rather than revisiting them.
• Refs feat(skill-quality): enforce frontmatter name matches skill directory name #712 — the sibling per-skill name-matches-directory gate.

[claude]

Claude finished @kyle-sexton's task in 2m 35s —— View job

---

PR Review: feat(ci): register cross-plugin skill leaf-name collisions

• Gather context and understand the request
• Read changed files
• Review checker script logic
• Review test script coverage
• Review registry file and CI integration
• Post findings

---

Overall this is a clean, well-reasoned addition. The architecture — discover mode, --check mode, stale guard, self-contained test harness — is correct. One factual inconsistency in the description, one UX nit. No blocking issues.

---

Correctness

Registry parsing is correct. The trimming idiom on lines 64–66 of check-skill-leaf-names.sh handles the three cases correctly: inline comments stripped via %%#*, leading/trailing whitespace removed via parameter expansion. Test case 5 ( shared # trailing comment) covers the full round-trip. The parser is intentionally POSIX-safe — no sed, no subshell per line.

The stale guard is the right call. Without it a renamed skill leaves a dangling entry that silently pre-authorizes the next collision on that name. Its test (case 6) verifies both the exit code and the error string.

((expr)) || continue with set -e is safe. In bash, || creates a compound command; the -e flag does not trigger when the left-hand side fails inside a compound. Lines 57 and 100 are both correct.

set -uo pipefail (no -e) in the test file is intentional. Omitting -e lets the harness capture exit codes from deliberately-failing SUT invocations. Without it every run --check that returns 1 would abort the test. The comment at the top of the file explains this.

---

One factual inconsistency

The CI step ordering in the new job is self-test → check, with the comment:

"Self-test first, so a broken detector cannot mask a regression."

That rationale is correct — if the detector is broken, the self-test fails loudly before the check can produce a silent false-pass.

However, the PR description claims this ordering follows "the established idiom" from the sibling job. It does not. The sibling cross-plugin-source-drift job runs check → self-test (check first, self-test second), the opposite order:

# cross-plugin-source-drift (existing)
- name: Check for unregistered or drifted cross-plugin source clusters
  run: scripts/check-cross-plugin-source-drift.sh --check        # ← check first
- name: Run cross-plugin-source-drift tests
  run: bash scripts/check-cross-plugin-source-drift.test.sh      # ← self-test second

# skill-leaf-name-gate (this PR)
- name: Run skill-leaf-name tests
  run: bash scripts/check-skill-leaf-names.test.sh               # ← self-test first
- name: Check for unregistered cross-plugin skill leaf-name collisions
  run: scripts/check-skill-leaf-names.sh --check                 # ← check second

The new ordering is arguably better (a broken detector is diagnosed immediately, not obscured by a passing check), but it is not mirroring the sibling — it inverts it. The description should either not invoke the sibling as the authority for this ordering, or the sibling should be updated to match. Worth a quick note in the PR or a follow-up to align the sibling.

See: .github/workflows/ci.yml L287–L298

---

Minor UX nit

check-skill-leaf-names.sh L89–90:

state="UNREGISTERED"
[[ -n "${registered[$leaf]:-}" ]] && state="registered"

UNREGISTERED is uppercase, registered is lowercase. The asymmetry may be intentional (uppercase draws the eye for the actionable state), but it's visually inconsistent in the discover output and could trip up anyone grepping the output programmatically. Since nothing currently scripts against discover mode this is truly minor — noting it for completeness.

---

No concerns

• Execute bits (100755) confirmed on both scripts.
• Pinned SHA on actions/checkout — consistent with all other jobs in the workflow.
• Job added to ci-status needs: list — the aggregate gate picks it up correctly.
• declare -A requires bash 4+; ubuntu-24.04 ships bash 5.2 — no issue.
• The shellcheck disable=SC2206 annotations on word-splitting are correctly scoped and annotated. Plugin names are kebab-case, so no splitting hazard in practice.
• Registry entries are clearly motivated; setup's rationale (non-consolidatable because ${CLAUDE_PLUGIN_ROOT} resolves per plugin) is the strongest and the most important to document for future auditors.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 420d0e12e2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude finished @kyle-sexton's task in 3m 2s —— View job

---

PR Review: feat(ci): register cross-plugin skill leaf-name collisions

• Gather context and understand the request
• Read changed files (full diff including fixup commit)
• Review checker script logic
• Review test script coverage
• Review registry file and CI integration
• Review fixup commit (owner-set registration)
• Post findings

---

The fixup commit (93241ee) correctly and completely resolves the Codex concern. The gate now rejects every new-owner scenario that the old bare-name format would have silently pre-authorized. Here's a focused review of what changed.

---

Fixup commit analysis

read -r leaf_key owner_field _ instead of word-splitting the trimmed line — correct fix.
The * in setup * would pathname-expand against the CWD if array-split with (${line}). Using read binds owner_field to the literal string *, which is what [[ "$accepted" == "*" ]] && continue needs.

owner_set() function — correct and POSIX-safe.
printf '%s\n' "$@" | sort | paste -sd, - emits one plugin name per line, sorts, then joins on commas. Both the discovered side and the registry side are normalized through it before comparison, so the check is genuinely set-vs-set and a hand-ordered entry in the registry doesn't fail on ordering alone. Test 5b covers this case explicitly.

${registered[$leaf]+set} to distinguish missing vs empty — correct.
This idiom is the right one here: an empty string (registered["shared"]="") represents a bare-name entry that needs rejection, not an absent key. The :- default would conflate the two. Lines 107 (discover) and 133 (check) are both right.

Empty owner-field path — correct but note the discover-mode signal.
When a registry line has no owner field (registered["$leaf"]=""), --check mode correctly reaches the [[ -z "$accepted" ]] branch and emits FAIL: registers shared without an owner set. In discover mode, the same entry falls into neither the [[ "$accepted" == "*" ]] nor the [[ -n "$accepted" ]] branch, so state stays OWNERS-CHANGED rather than something like NO-OWNERS. Not a correctness issue — the discover output is informational and --check handles it precisely — but a future reader grep-ing for OWNERS-CHANGED may find entries in two different situations. Low priority.

The SC2310 disable annotations (owner_set only sorts strings; nothing inside can fail) are accurate — sort and paste -sd, can only fail on I/O, not on valid string input — and correctly scoped to the individual subshell invocations.

---

Test coverage

Twelve named assertions; the new owner-set scenarios are fully covered:

Case	What it tests
5b	Order-independent set comparison (gamma,alpha,beta ≡ alpha,beta,gamma)
5c	Owner set grew → fails
5d	Bare-name entry (old format) → fails with without an owner set
5e	* wildcard passes and doesn't glob-expand to cwd entries

The gap from the previous review — no test for OWNERS-CHANGED in discover mode — remains, but that's informational output on a path that --check tests correctly (5c).

---

Confirmed from previous review

CI step ordering (PR description inconsistency) — still present, still minor.
The sibling job (cross-plugin-source-drift) runs check → self-test; this job runs self-test → check. The PR description says the new ordering "follows the established idiom" of the sibling, which is factually wrong — it inverts it. The new ordering is arguably correct on the merits (a broken detector fails loudly before a silent false-pass is possible), but the description's authority claim doesn't hold. Worth a note for the record, not a merge blocker.

Capitalization in discover output — now coherent.
UNREGISTERED and OWNERS-CHANGED (uppercase) signal action needed; registered and registered(*) (lowercase) signal accepted. The asymmetry is now a design choice, not an accident. The previous nit no longer applies.

---

No concerns

• Execute bits confirmed on both scripts.
• declare -A requires bash 4+; ubuntu-24.04 ships 5.2.
• Pinned SHA on actions/checkout, consistent with other jobs.
• skill-leaf-name-gate is in ci-status needs: list — aggregate gate picks it up.
• Registry entries carry clear rationale; setup * (open owner set by contract) is the most important to document and it is.
• The stale guard and the owner-set mismatch guard compose correctly: both loops run and accumulate failed=1 before exit, so a registry with both a stale entry and a mismatched entry reports both failures in one pass.

Overall: Clean, well-reasoned, and the fixup is correct. The Codex concern is fully addressed. Ready to merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 93241eeaec

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude finished @kyle-sexton's task in 1m 49s —— View job

---

PR Review: feat(ci): register cross-plugin skill leaf-name collisions

• Gather context and understand the request
• Read changed files (focusing on latest fixup commit 59ee8db)
• Verify zero-collision fix (Codex P2 concern)
• Review overall state of the PR
• Post findings

---

This is a focused review of the latest fixup commit (59ee8db fix(ci): survive the zero-collision end state), which is the only outstanding concern after the prior two review passes.

---

Zero-collision fix — correct and complete

The Codex P2 concern was: with set -u active, ${#collisions[@]} on an associative array that was declared but never assigned would abort with unbound variable. This is the exact state the stale-entry guard is designed to reach — making it the one state that crashed the checker.

The fix introduces two shadow variables:

# scripts/check-skill-leaf-names.sh L64–74
declare -A collisions
collision_leaves=()        # plain indexed array — safe under set -u when empty
collision_count=0          # integer — always bound
for leaf in "${!leaf_owners[@]}"; do
  ((${#owners[@]} >= 2)) || continue
  collisions["$leaf"]="${leaf_owners[$leaf]}"
  collision_leaves+=("$leaf")
  collision_count=$((collision_count + 1))
done

Every downstream reference is updated consistently:

• Discover-mode loop: ${collision_leaves[@]+"${collision_leaves[@]}"} — the + conditional expansion returns nothing when the array is empty, safely producing zero iterations.
• --check loop: same idiom, L139.
• Stale guard: ${registered_leaves[@]+"${registered_leaves[@]}"} — same pattern applied to the registry side.
• Success message: $collision_count instead of ${#collisions[@]}.
• Discover-mode early-exit: if ((collision_count == 0)) — prints "No cross-plugin skill leaf-name collisions." and exits 0 cleanly.
• Stale guard's per-leaf lookup: ${collisions[$leaf]:-} uses :- (default expansion) rather than +set, which is correct here: the stale guard is walking registered_leaves and checking whether each name is still in collisions; the :- default gracefully returns empty when the key is absent, letting the FAIL path proceed.

The fix is mechanically complete. Every path that could touch ${#collisions[@]} or ${!collisions[@]} has been rerouted through the shadow variables.

---

Test coverage for the fix — thorough

Test 7b builds a separate clean fixture tree (no collisions, empty registry) and verifies both modes:

Assertion	What it checks
rc == 0 on --check	gate exits cleanly, not with unbound variable
!grep -q 'unbound variable'	no unexpected stderr bleed
rc == 0 on discover	same safety in the other mode
grep -q 'No cross-plugin skill leaf-name collisions'	the human-readable message lands correctly

The clean fixture is isolated in its own mktemp -d ($CLEAN) so the existing three-way collision fixture ($TMP) in tests 1–7 is not contaminated. rm -rf "$CLEAN" runs inline before test 8, keeping the cleanup predictable even if the trap fires concurrently.

---

Remaining note (documentation only, not a merge blocker)

The CI step comment says:

"Self-test first, so a broken detector cannot mask a regression."

That rationale is correct. The PR description says this ordering follows "the established idiom" of the sibling cross-plugin-source-drift job, but the sibling runs check-first, self-test-second — the opposite order. The new ordering is arguably better on the merits, but the description's authority claim doesn't hold. Worth a one-line correction in the PR description or a follow-up to align the sibling; not a reason to hold the merge.

---

Summary

The zero-collision crash is fixed, the fix is applied consistently across every affected path, test 7b covers both failure directions for that state, and the grammar introduced (collision_leaves + collision_count shadows) is clearly commented. All concerns from prior rounds are resolved. Ready to merge.