fix(core): stop skill discovery from descending into skill directories

[archibate]

Fixes #1304

Problem

discoverSkillsInDir recursively registers every SKILL.md it finds at any depth. Skills that bundle SKILL.md-formatted reference templates leak all of them as commands: e.g. a frontend-design skill shipping ~100 style recipes under references/styles/<style>/SKILL.md floods the Discord slash-command list with finance-report, dcf-valuation, 40+ html-ppt-*, etc. — commands the user never installed, which execute out-of-context template prompts when tapped. Claude Code itself treats anything below a skill directory as private assets.

Fix

A directory containing SKILL.md is a skill — register it and stop descending: deeper SKILL.md files are that skill's internal assets, not standalone skills.

Grouped layouts keep working: descent only stops once a SKILL.md is found, and group folders (<root>/automation/<skill>/SKILL.md) have none, so all existing tests pass unchanged (grouping, symlinks, loop protection, dedupe, root SKILL.md ignore).

Tests

• New regression test TestSkillRegistryListAll_IgnoresNestedSkillFilesInsideSkillDirectories: a skill with nested reference templates resolves only the skill itself; the templates do not resolve.
• All 5 pre-existing skill registry tests pass unchanged.
• Full go test ./core/ passes.

🤖 Generated with Claude Code

[chenhg5]

Conclusion: Comment

Overall assessment:

• This PR is fully SUPERSEDED by PR #1317 (merged to main as commit cfb080f, "fix(skill): scan only depth-1 SKILL.md, ignore nested files (#1304)"). Same bug, same fix intent, simpler implementation, already on main. I would recommend closing this PR.

Review scope:

• Reviewed core/skill.go (+22/-16) on worktree qa/pr-1306-verify @ d5d0544, base origin/main c53f545.
• Cross-referenced against main (commit cfb080f) which is the in-main fix for the same issue.
• Focused on correctness, scope/duplication, and mergeability.

✅ What looks good:

• The root-cause analysis is right: nested SKILL.md files inside a skill's private reference assets leak into platform command menus as phantom slash commands. The example in the PR body (frontend-design/references/styles/<style>/SKILL.md → 101 phantom commands) is exactly the bug report #1304.
• The fix correctly identifies that "a directory containing SKILL.md is a skill, and once you find that SKILL.md, deeper SKILL.md files are that skill's private assets". The PR keeps grouped layouts (<root>/automation/<skill>/SKILL.md) working since the descent-stop only fires when a SKILL.md is found in the current dir.
• The new regression test TestSkillRegistryListAll_IgnoresNestedSkillFilesInsideSkillDirectories covers the exact scenario from the bug report. The 5 pre-existing skill tests pass unchanged. CI run 27351254992 5/5 green on this PR.

❌ Strong recommendation — close as superseded:

• PR #1317 is on main and does the same job, but in a more aggressive way. Where your PR keeps discoverSkillsInDir recursive and just adds an early-return when a SKILL.md is found in currentDir, #1317 rewrote discoverSkillsInDir to be depth-1 only — discoverSkillsInDir(scanRoot, seen) with a single os.ReadDir(scanRoot) and a per-entry isDiscoverableSubdir filter. Same outcome, ~30 fewer lines, no visited map, no realPath indirection, and the helper function signatures match what core/skill_test.go already wanted.
• All three of the symbols you introduce here are now absent from main: sameFilePath(currentDir, scanRoot) early-return branch, the "not a skill directory itself — keep descending" comment block, and the isDiscoverableSubdir helper that #1317 added (your version of it is inlined differently). Functionally the depth-1 stop is the same.
• Merging this PR on top of #1317 would conflict (both rewrite the discoverSkillsInDir body and the seen map signature changes from map[string]bool, map[string]bool to just map[string]bool). It would also be a strict functional no-op: any test that passes with #1317's depth-1 layout also passes with your "recursive + early-return" implementation, because #1317's stop condition is strictly stronger (it never descends at all once the first SKILL.md is hit; yours stops at that directory but doesn't prevent descent into siblings). Same observable behaviour.
• One genuine difference I want to flag for the maintainer's awareness: #1317's commit message explicitly says "matches the Claude Code CLI convention" and is a behavioral specification as much as a bug fix. Your PR description frames the fix as "a directory containing SKILL.md is a skill" which is a useful invariant statement but does not call out the CLI compatibility angle. The two fixes are aligned in outcome; just noting the framing difference in case it matters for the dev conversation.

Testing / Risk:

• Verified evidence: CI run 27351254992 5/5 green on this PR (lint 2m41s, unit-test 4m09s, smoke 31s, regression 25s, performance 45s). gofmt -l core/skill.go clean. The fix's key idea (early-return when SKILL.md is in currentDir) is functionally a strict subset of #1317's depth-1 implementation. 0 SUPERSEDED hits via git grep for the symbols themselves because #1317's rewrite is structured differently, but the user-observable behavior is the same.
• Remaining risk: If this PR is closed cleanly, none. If a maintainer tries to rebase and merge, the result is functionally equivalent to #1317 but with merge conflict resolution work for no benefit. Pick the close.

Next step:

• Owner decision: close this PR as superseded by #1317 (commit cfb080f on main). No code from this PR needs to be ported — the equivalent fix is already in production. If you want the regression test from this PR (TestSkillRegistryListAll_IgnoresNestedSkillFilesInsideSkillDirectories) preserved, check whether #1317's test suite already covers the same scenario; if not, a follow-up PR adding the regression test against the new discoverSkillsInDir(scanRoot, seen) signature would be a quick +1.

[chenhg5]

Conclusion: Request changes

Overall assessment:

• This PR is fully SUPERSEDED by PR #1317 (merged to main as commit cfb080f, "fix(skill): scan only depth-1 SKILL.md, ignore nested files (#1304)"). Same bug, same fix intent, simpler implementation, already on main. I would recommend closing this PR.

Review scope:

• Reviewed core/skill.go (+22/-16) on worktree qa/pr-1306-verify @ d5d0544, base origin/main c53f545.
• Cross-referenced against main (commit cfb080f) which is the in-main fix for the same issue.
• Focused on correctness, scope/duplication, and mergeability.

✅ What looks good:

• The root-cause analysis is right: nested SKILL.md files inside a skill's private reference assets leak into platform command menus as phantom slash commands. The example in the PR body (frontend-design/references/styles/<style>/SKILL.md → 101 phantom commands) is exactly the bug report #1304.
• The fix correctly identifies that "a directory containing SKILL.md is a skill, and once you find that SKILL.md, deeper SKILL.md files are that skill's private assets". The PR keeps grouped layouts (<root>/automation/<skill>/SKILL.md) working since the descent-stop only fires when a SKILL.md is found in the current dir.
• The new regression test TestSkillRegistryListAll_IgnoresNestedSkillFilesInsideSkillDirectories covers the exact scenario from the bug report. The 5 pre-existing skill tests pass unchanged. CI run 27351254992 5/5 green on this PR.

🔴 Must fix (Request changes — the resolution here is "close the PR", not "make a code change"):

• P0: PR #1317 is on main and does the same job, but in a more aggressive way. Where your PR keeps discoverSkillsInDir recursive and just adds an early-return when a SKILL.md is found in currentDir, #1317 rewrote discoverSkillsInDir to be depth-1 only — discoverSkillsInDir(scanRoot, seen) with a single os.ReadDir(scanRoot) and a per-entry isDiscoverableSubdir filter. Same outcome, ~30 fewer lines, no visited map, no realPath indirection, and the helper function signatures match what core/skill_test.go already wanted.
  Impact: If merged, the codebase will have two parallel implementations of the same fix. Future maintenance / refactoring of skill discovery has to reason about both. The behavior is the same; the cost is duplicated code and a noisy git history.
  Evidence: git show origin/main:core/skill.go | head -90 shows the depth-1 rewrite on main. git show cfb080f9 -- core/skill.go shows the same logical transformation.
  Suggested fix: Close this PR as superseded by #1317 (commit cfb080f on main). No code from this PR needs to be ported — the equivalent fix is already in production. If the maintainer wants the regression test from this PR preserved, verify whether #1317's test suite already covers the same scenario; if not, a follow-up PR adding the regression test against the new discoverSkillsInDir(scanRoot, seen) signature would be a quick +1.

🟠 Should improve:

• P1: Merging this PR on top of #1317 would conflict. Both rewrite the discoverSkillsInDir body and the seen map signature changes from map[string]bool, map[string]bool to just map[string]bool. Resolving the conflict would be busywork for no behavioral benefit (the outcome is identical to #1317 already in place).
  Impact: Maintainer has to do a no-op merge conflict resolution.
  Suggested fix: Same as above — close.

❓ Question:

• One genuine difference I want to flag for the maintainer's awareness: #1317's commit message explicitly says "matches the Claude Code CLI convention" and frames the fix as a behavioral specification as much as a bug fix. Your PR description frames the fix as "a directory containing SKILL.md is a skill" which is a useful invariant statement but does not call out the CLI compatibility angle. The two fixes are aligned in outcome; just noting the framing difference in case it matters for the dev conversation.

Testing / Risk:

• Verified evidence: CI run 27351254992 5/5 green on this PR (lint 2m41s, unit-test 4m09s, smoke 31s, regression 25s, performance 45s). gofmt -l core/skill.go clean. The fix's key idea (early-return when SKILL.md is in currentDir) is functionally a strict subset of #1317's depth-1 implementation. 0 SUPERSEDED hits via git grep for the symbols themselves because #1317's rewrite is structured differently, but the user-observable behavior is the same.
• Remaining risk: If this PR is closed cleanly, none. If a maintainer tries to rebase and merge, the result is functionally equivalent to #1317 but with merge conflict resolution work for no benefit. Pick the close.

Next step:

• Owner decision: close this PR as superseded by #1317 (commit cfb080f on main). No code from this PR needs to be ported — the equivalent fix is already in production.