fix(skill-quality): name the sibling skill when a cross-skill citation misses

[kyle-sexton]

Closes #2173

Summary

check-skill.sh check 5 resolves every bare context/…-shaped path against the citing skill's
own directory. A skill citing a sibling skill's supporting file therefore failed as a broken
internal ref — "no such file under the skill dir" — while the file plainly existed one directory
over, and the message pointed at the citing skill. The author's natural next move was to look for
the file where it could never be. That misdirection cost two round trips in a single PR (#2161),
once in each direction.

This implements issue options (2) message and (1) docs. Option (3) (narrowing extraction to
link targets) is deliberately not implemented: prose and inline-code refs are presumed-intended
coverage, and dropping them is a separate call.

Fix

• Sibling-aware diagnosis (check 5). When the unresolved path also resolves under a sibling
  skill of the same skills root, the finding names that sibling and the citation form that works.
  Still a FAIL — the bare form really does resolve against the citing skill, so it is wrong
  regardless of where the file lives; only the diagnosis gains detail.

• The sibling hit is evidence, not proof. Because this check deliberately extracts prose and
  inline-code refs, a generic path (scripts/run.sh) can collide with an unrelated same-named
  sibling file. The original message — including its hand-verify the line before fixing, may be an illustrative example instruction — is kept verbatim, and the sibling evidence is appended
  conditionally, so both readings stay live:

  FAIL: broken skill-internal ref: context/suppression.md (no such file under the skill dir; cited
  at SKILL.md:7 — hand-verify the line before fixing, may be an illustrative example). A file with
  that path DOES exist under sibling skill '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
  ${CLAUDE_PLUGIN_ROOT}/skills/host/context/suppression.md. If the names merely collide, the ref is
  unrelated to that sibling.
  

• Layout-aware suggestion. Which form is correct depends on the skills root, and the script's
  resolution ladder deliberately bakes no layout. A plugin-shaped root gets
  ${CLAUDE_PLUGIN_ROOT}/skills/<sibling>/<path>; outside a plugin that variable is undefined, so
  the message names the layout-free ../<sibling>/<path> instead of advertising a variable the
  consumer does not have. The plugin-shaped test itself is hoisted to a single IS_PLUGIN_SKILL
  computed once after the skill dir resolves — check 1's bare-alias branch and check 5's suggested
  form now read one boolean instead of restating the same layout convention twice.

• No false positives. A path no sibling hosts keeps the original message unchanged. Sibling
  enumeration reuses check 3's "$SKILLS_ROOT"/*/SKILL.md glob (sorted, so a path present under
  more than one sibling names the first deterministically) and skips the citing skill.

• The two grep generators feeding the loop are untouched — extraction behavior is unchanged.

• Docs (docs/PLUGIN-PHILOSOPHY.md). Two sentences in the existing Configuration ownership and
  scope section, directly parallel to the ${CLAUDE_PROJECT_DIR} project-path rule already there
  and to the section's own Bundled plugin code and assets | ${CLAUDE_PLUGIN_ROOT} row. No new
  section. plugins/skill-quality/README.md's check-5 bullet and the script's own header list are
  updated to match.

Verification

• plugins/skill-quality/scripts/check-skill.test.sh — three new cases, all confirmed passing in
  CI (plugin-gate → PASS: plugins/skill-quality/scripts/check-skill.test.sh):
  • 36a: plugin-shaped root names the host sibling and the ${CLAUDE_PLUGIN_ROOT} form, and
    still carries hand-verify the line — the caveat cannot be silently dropped again.
  • 36b: non-plugin root names the sibling-relative ../<sibling>/<path> form and not the
    plugin-root one.
  • 36c: a ref no sibling hosts keeps the plain broken-internal-ref message with no sibling skill
    text (false-positive guard).
• Manual reproduction against a throwaway plugin-shaped fixture reproduces the message quoted above,
  while a genuinely absent ref in the same SKILL.md still reports the unmodified
  broken skill-internal ref: context/absent-file.md (…).
• Repo gates, base-ref form against the committed tree: check-shell-portability.sh,
  check-skill-portability.sh, check-changelog-parity.sh --check-bump, check-changed-skills.sh,
  check-contract-slice-prune.sh --check-diff — all pass. markdownlint-cli2 clean on the changed
  markdown. shellcheck clean and shfmt -i 2 clean on both changed shell files.
• Coverage note: the full suite was run to completion on Linux CI, not locally on Windows/Git Bash
  (where it is impractically slow); local Windows confirmation is limited to targeted smoke tests of
  the two new message branches. Nothing in the change is platform-sensitive — pure bash string ops
  and -e/-f tests, no date/readlink/GNU-only constructs — and shell-portability-lint passes.
• plugins/skill-quality bumped to 0.15.1 with a matching ## [0.15.1] CHANGELOG entry (patch is
  consistent with this plugin's history: 0.12.1 shipped a whole new check as a patch).

Related

• Closes check-skill.sh: a cross-skill reference fails as a broken internal ref, and the error points at the wrong directory #2173
• feat(mutation-testing): add the mutation-testing plugin and wire the measurement into tdd and verification #2161 — the PR where the misfire was hit twice, once in each direction
• fix(session-flow,discipline): drop the per-session subagent cap that no longer exists, and record the doc-currency audit #2169 — the gate doc-currency audit that recorded the finding

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[claude]

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

---

Security review complete

• Gather context (PR diff, changed files)
• Review plugins/skill-quality/scripts/check-skill.sh changes for injection / logic / path-handling issues
• Review plugins/skill-quality/scripts/check-skill.test.sh changes
• Review docs/README/CHANGELOG/plugin.json changes
• Post findings

This PR only touches a local, static shell-based lint script for skill-authoring quality (check-skill.sh), its test fixtures, and docs/metadata. No workflow files, no network calls, no secrets handling. I found no security issues in this diff.

Notes from what I checked (not findings, just what was verified as safe):

• No injection surface introduced. The new sibling-lookup loop (plugins/skill-quality/scripts/check-skill.sh lines ~366–393) reuses the same $ref value already extracted by the untouched grep generators at the bottom of the check-5 block. Those generators constrain matches to [A-Za-z0-9._/#-] — no shell metacharacters, $(), backticks, or quotes can appear in $ref, so the new [[ -e "${other_md%/SKILL.md}/$ref" ]] existence tests and the new err "... $REF_HOST/$ref" message strings can't be leveraged for command injection or format-string tricks (err/warn/note all use printf '%s\n' "$*", a fixed format string).
• No new path-traversal exposure. The character class does allow . and /, so a ../-laden ref could in principle test file existence outside the skill dir — but that's pre-existing behavior of check 5's original bare [[ ! -e "$SKILL_DIR/$ref" ]] test (untouched by this PR), and the new code only extends the same existence-only check (no content is read or echoed back) to sibling directories under the same, repo-local $SKILLS_ROOT. Nothing about the trust boundary changes here.
• Sibling enumeration is safe. for other_md in "$SKILLS_ROOT"/*/SKILL.md is a quoted glob with no eval, and plugin.json detection reuses check 1's existing, unmodified logic.
• Test, docs, README, CHANGELOG, and plugin.json version-bump changes are inert (fixtures under a temp dir, prose, and a semver bump) — nothing security-relevant.

Per instructions, I deferred to zizmor's static lane for any workflow/supply-chain/permissions concerns — none of those surfaces are touched by this PR anyway (no .github/workflows/* files changed).

Branch: work/2173-auto

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7e60197baa

ℹ️ 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 4m 10s —— View job

---

Review complete

• Gather context (diff, changed files, CLAUDE.md/REVIEW.md guidelines)
• Understand the request (code review of correctness + alignment with CLAUDE.md)
• Review plugins/skill-quality/scripts/check-skill.sh changes
• Review plugins/skill-quality/scripts/check-skill.test.sh new tests
• Review docs updates (docs/PLUGIN-PHILOSOPHY.md, README.md, CHANGELOG.md, plugin.json version bump)
• Post inline comments for findings
• Post final summary

Scope note: this repo has .github/workflows/claude-security-review.yml, so per REVIEW.md this lane leaves security findings to that lane. Nothing in this diff touches auth/authorization/secrets/injection anyway — it's a bash static-analysis check plus docs.

What the change does

Check 5 in check-skill.sh resolves every bare context/…-shaped path against the citing skill's own directory. When a ref misses there, the fix now scans sibling skill directories under the same skills root (reusing check 3's "$SKILLS_ROOT"/*/SKILL.md glob pattern) before falling back to the original "broken ref" message. If a sibling hosts the file, the FAIL message names that sibling and the correct citation form — ${CLAUDE_PLUGIN_ROOT}/skills/<sibling>/<path> in a plugin-shaped root (reusing check 1's plugin detection), or ../<sibling>/<path> outside one. A path no sibling hosts keeps the original wording verbatim, so there's no new false-positive surface.

Correctness

Traced the new branch (plugins/skill-quality/scripts/check-skill.sh:366-398) end to end:

• The [[ "$other_md" == "$SKILL_MD" ]] self-skip and the sorted-glob "first match wins" behavior both mirror the existing pattern in check 3 (lines 296–298, 318–319), so this isn't a new assumption for the script.
• The plugin-vs-non-plugin branch reuses check 1's exact detection expression, and the ${CLAUDE_PLUGIN_ROOT}/skills/... form is consistent with the fixed skills/ directory convention used by every plugin in this repo (confirmed via grep — 112 hits across plugins/*/skills/... and existing ${CLAUDE_PLUGIN_ROOT}/skills references) — so hard-coding skills/ in the message doesn't fight the "no baked layout" design of the SKILLS_ROOT resolution ladder; that ladder is about locating the citing skill under an arbitrary repo layout, not about the plugin skills-directory spec itself.
• The non-plugin fallback ../$REF_HOST/$ref is a relative path from the citing skill's own directory to the sibling's — correct given SKILL_DIR and the sibling are both direct children of SKILLS_ROOT, and consistent with the new docs/PLUGIN-PHILOSOPHY.md example pairing the plugin-root form with a relative link for GitHub browsing.
• Traced test cases 36a/36b/36c by hand against the fixture setup (including reused PLUGIN_SKILLS/plugin.json fixture from test 34): each assertion matches what the code actually produces. No gaps found in the plugin / non-plugin / no-sibling-hosts coverage.

One inline suggestion posted (maintainability, not a correctness bug): the plugin-shaped-root test (-f "$SKILL_DIR/../../.claude-plugin/plugin.json") is now duplicated verbatim at two call sites (check 1 and check 5) with no shared source of truth — see inline comment.

Docs / changelog / version

docs/PLUGIN-PHILOSOPHY.md, plugins/skill-quality/README.md, and the script's own header list all describe the new behavior consistently and match what the code does. CHANGELOG.md's new ## [0.15.1] entry matches the plugin.json version bump (0.15.0 → 0.15.1) and the Keep a Changelog format used elsewhere in the file.

What I could not independently verify

This session's sandbox blocked executing shellcheck, shfmt, and the test suite itself (check-skill.test.sh) — those tool invocations were denied approval. So the PR body's claims of a clean shellcheck/shfmt pass and a full green test run are author-claimed and unverified by me; I did read the shell diff and the new test cases closely by hand instead, per above. If you'd like independent execution confirmation, that needs to run in an environment with those tools permitted.

No blocking issues found. This is a solid, well-scoped fix with good test coverage for the plugin / non-plugin / no-sibling-hosts cases.

[github-actions]

Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

[claude]

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

---

Review complete

• Gather context (diff via gh pr diff, since this sandbox's shallow clone has no origin/main merge base and git fetch requires interactive approval unavailable here)
• Review plugins/skill-quality/scripts/check-skill.sh changes (sibling-aware diagnosis + hoisted IS_PLUGIN_SKILL)
• Review plugins/skill-quality/scripts/check-skill.test.sh new tests (36a/36b/36c)
• Review docs (docs/PLUGIN-PHILOSOPHY.md), README, CHANGELOG, plugin.json version bump
• Post findings

Scope note: per REVIEW.md, this repo has .github/workflows/claude-security-review.yml, so this lane leaves security findings to that lane. Nothing here touches auth/secrets/injection anyway.

This is the follow-up commit — both round-1 findings are addressed

Commit 3f58e20 (on top of 7e60197) directly answers both pieces of feedback from the first review pass:

• Codex's ambiguity concern (comment on check-skill.sh:401, "preserve ambiguity when sibling paths collide") — the sibling-hit message no longer asserts the sibling is the target. It now reads "if that is the file meant, this is a cross-skill citation... If the names merely collide, the ref is unrelated to that sibling" (check-skill.sh:417), and the original hand-verify wording is kept verbatim rather than replaced. This closes the exact failure mode Codex flagged (a generic path like scripts/run.sh colliding with an unrelated sibling file).
• My own round-1 suggestion (hoist the duplicated plugin-detection test) — IS_PLUGIN_SKILL is now computed once at check-skill.sh:209-210, placed right after the SKILL_MD existence check (before check 1 needs it), and both check 1 (line 244) and check 5 (line 412) now reuse the single boolean instead of re-running the -f .../plugin.json test.

Correctness (traced by hand)

• IS_PLUGIN_SKILL and the sibling-scan loop reuse existing, already-battle-tested patterns rather than introducing new ones: the for other_md in "$SKILLS_ROOT"/*/SKILL.md glob, the [[ -f "$other_md" ]]/self-skip guards, and the "sorted glob → first match wins" behavior are byte-for-byte the same shape as check 3's existing sibling loop (check-skill.sh:306-308), so this isn't a new assumption for the script.
• ${other_md%/SKILL.md}/$ref correctly reduces to $SKILLS_ROOT/$dir/$ref — verified against how SKILL_DIR/SKILL_MD are built at the top of the script (SKILL_DIR="$SKILLS_ROOT/$SKILL_NAME", SKILL_MD="$SKILL_DIR/SKILL.md").
• The plugin/non-plugin branch producing ${CLAUDE_PLUGIN_ROOT}/skills/$REF_HOST/$ref vs ../$REF_HOST/$ref is correct given SKILL_DIR and the sibling are both direct children of SKILLS_ROOT.
• Verified the three new test fixtures (36a/36b/36c) actually exercise what they claim: 36a's PLUGIN_SKILLS root correctly has $TMP/plugins/demo/.claude-plugin/plugin.json two levels up (reused from the earlier test-34 fixture), so IS_PLUGIN_SKILL=1 is genuinely exercised; 36b/36c's $SKILLS root ($TMP/.claude/skills) has no .claude-plugin/plugin.json two levels up anywhere in the fixture tree, so the non-plugin branch is genuinely exercised too — this isn't a test asserting a branch it never actually reaches.
• 36c (no sibling hosts the ref) correctly falls through to the original, unmodified broken skill-internal ref message with no sibling skill text — confirms the false-positive guard works.

No correctness issues found in this diff. Docs (docs/PLUGIN-PHILOSOPHY.md), README's check-5 bullet, the script's own header comment, and the CHANGELOG.md/plugin.json version bump (0.15.0 → 0.15.1) are all internally consistent with what the code does.

Not independently verified

I ran no commands in this session beyond reading the diff and source by hand (gh pr diff, Read, Grep) — this sandbox couldn't reach origin/main via git fetch (shallow clone, needs interactive approval unavailable here) and I did not execute shellcheck, shfmt, or check-skill.test.sh. The PR body's claims of clean shellcheck/shfmt and a full green test run are author-claimed and unverified by me — I instead traced the shell diff and the three new test cases by hand, per above.

[claude]

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

---

Security review complete

• Gather context (gh/git diff origin/main...HEAD, changed files)
• Review plugins/skill-quality/scripts/check-skill.sh changes for injection / access-control / path-handling issues
• Review plugins/skill-quality/scripts/check-skill.test.sh changes
• Review docs/README/CHANGELOG/plugin.json changes
• Post findings

No security issues found. This PR only touches a local, static shell-based lint script for skill-authoring quality (check-skill.sh), its test fixtures, and docs/metadata — no workflow files, no network calls, no secrets handling, no change to trust boundaries.

Verified independently on the current head (3f58e20), which is one commit ahead of what the earlier automated pass on this PR reviewed:

• No injection surface. The new sibling-lookup loop (check-skill.sh#L378-L416) consumes $ref, which is produced by the two grep generators at the bottom of the check-5 block — both untouched by this PR and constrained to [A-Za-z0-9._/#-]+ (confirmed by re-reading check-skill.sh#L422-L424): no shell metacharacters, $(), backticks, or quotes can appear in it. err()/warn()/note() all call printf with a fixed format string ('FAIL: %s\n', "$*" as the argument, not the format) — confirmed at check-skill.sh#L158-L170 — so the new err "... $REF_HOST/$ref" messages can't be leveraged for format-string or command-injection tricks.
• No new path-traversal exposure. The allowed character class does include . and /, so a crafted ../-laden ref could in principle test file existence outside the skill dir — but that's pre-existing behavior of check 5's original bare [[ ! -e "$SKILL_DIR/$ref" ]] test, unmodified here. The new code ([[ -e "${other_md%/SKILL.md}/$ref" ]]) only extends the same existence-only check to sibling directories under the same, repo-local $SKILLS_ROOT — nothing is read, catted, or executed, only a boolean existence result feeds into the diagnostic string. No trust boundary is crossed: SKILL.md content is authored locally by the same developer running the linter, not attacker-controlled input arriving over any external boundary.
• Sibling enumeration is safe. for other_md in "$SKILLS_ROOT"/*/SKILL.md is a quoted glob, no eval, no word-splitting risk from unquoted expansion. IS_PLUGIN_SKILL plugin-shape detection (check-skill.sh#L203-L209) is a plain -f test against a fixed relative path, now hoisted to one call site and reused by checks 1 and 5 (addressing the maintainability point raised in the prior review pass) — no behavior change from the duplicated version.
• Test, docs, README, CHANGELOG, and plugin.json version-bump changes are inert (fixtures under a $TMP temp dir, prose, and a semver bump) — nothing security-relevant.

No .github/workflows/* files are touched by this PR, so zizmor's static lane (supply-chain, unpinned actions, dangerous triggers, permissions) has nothing to add here either — this finding set is scoped to the logic/data-flow/trust-boundary reasoning called out for this lane, per repo instructions.

Branch: work/2173-auto