diff --git a/scripts/generate-skill-index.py b/scripts/generate-skill-index.py index 1bc7933c..88f0fdca 100755 --- a/scripts/generate-skill-index.py +++ b/scripts/generate-skill-index.py @@ -430,6 +430,8 @@ def _process_skill_dir(skill_dir: Path, skill_file_override: Path | None = None) old_ok = (repo_root / existing["file"]).is_file() if repo_root else False if old_ok and not new_ok: return + if old_ok and new_ok: + warnings.append(f" - {name}: duplicate skill name — {existing['file']} overwritten by {entry['file']}") index[collection_key][name] = entry for child in sorted(source_dir.iterdir()): @@ -443,6 +445,16 @@ def _process_skill_dir(skill_dir: Path, skill_file_override: Path | None = None) # Check if this directory directly contains a SKILL.md (flat layout) if (child / "SKILL.md").exists(): _process_skill_dir(child) + # Hybrid dirs (e.g. skills/process/, skills/research/) have both a top-level SKILL.md and nested skill subdirs. + for nested in sorted(child.iterdir()): + if not nested.is_dir(): + continue + if nested.is_symlink() and not include_private: + continue + if (nested / "SKILL.md").exists(): + _process_skill_dir(nested) + elif (nested / "skill" / "SKILL.md").exists(): + _process_skill_dir(nested, skill_file_override=nested / "skill" / "SKILL.md") # Check for nested skill/SKILL.md layout (e.g., voice-example/skill/SKILL.md) elif (child / "skill" / "SKILL.md").exists(): _process_skill_dir(child, skill_file_override=child / "skill" / "SKILL.md") diff --git a/scripts/tests/test_generate_skill_index.py b/scripts/tests/test_generate_skill_index.py index 7b115e61..8d2e6934 100755 --- a/scripts/tests/test_generate_skill_index.py +++ b/scripts/tests/test_generate_skill_index.py @@ -376,3 +376,56 @@ def test_include_private_run_writes_only_the_requested_output(self, tmp_path: Pa data = json.loads(output.read_text(encoding="utf-8")) # Floor tracks the post-consolidation catalog; raise it with new skills. assert 35 <= len(data["skills"]) <= 80 + + +class TestHybridDirIndexing: + """Hybrid dirs (top-level SKILL.md + nested skill subdirs) appear fully in index.""" + + def test_hybrid_dir_top_and_nested_both_indexed(self, tmp_path: Path) -> None: + """A hybrid dir's top-level skill and its nested skill both appear in the index.""" + skills_dir = tmp_path / "skills" + # Top-level hybrid skill (e.g. skills/process/SKILL.md) + process_dir = skills_dir / "process" + process_dir.mkdir(parents=True) + (process_dir / "SKILL.md").write_text(_SKILL_FRONTMATTER.format(name="process")) + # Nested skill under the hybrid dir (e.g. skills/process/pr-workflow/SKILL.md) + nested_dir = process_dir / "pr-workflow" + nested_dir.mkdir() + (nested_dir / "SKILL.md").write_text(_SKILL_FRONTMATTER.format(name="pr-workflow")) + + index, _warnings = gsi.generate_index( + source_dir=skills_dir, + dir_prefix="skills", + collection_key="skills", + ) + + assert "process" in index["skills"], "Top-level hybrid skill must be indexed" + assert "pr-workflow" in index["skills"], "Nested skill in hybrid dir must be indexed" + + +class TestOverwriteWarning: + """A duplicate skill name emits an overwrite warning.""" + + def test_duplicate_name_emits_warning(self, tmp_path: Path) -> None: + """Two skill dirs with the same name: in frontmatter triggers the overwrite-warning branch.""" + skills_dir = tmp_path / "skills" + # Two different dir names, same name: in frontmatter so both map to "shared-name". + dir_a = skills_dir / "cat-a" / "shared-skill" + dir_a.mkdir(parents=True) + (dir_a / "SKILL.md").write_text(_SKILL_FRONTMATTER.format(name="shared-name")) + + dir_b = skills_dir / "cat-b" / "shared-skill" + dir_b.mkdir(parents=True) + (dir_b / "SKILL.md").write_text(_SKILL_FRONTMATTER.format(name="shared-name")) + + index, warnings = gsi.generate_index( + source_dir=skills_dir, + dir_prefix="skills", + collection_key="skills", + repo_root=tmp_path, + ) + + assert "shared-name" in index["skills"], "The skill must still be indexed" + assert any("shared-name" in w for w in warnings), ( + f"Expected an overwrite warning for 'shared-name'; got warnings: {warnings}" + ) diff --git a/scripts/validate-skill-names.py b/scripts/validate-skill-names.py index 5d8db5c2..fd3643c0 100644 --- a/scripts/validate-skill-names.py +++ b/scripts/validate-skill-names.py @@ -38,6 +38,12 @@ def find_duplicates(repo: Path) -> dict[str, list[str]]: continue if (top / "SKILL.md").is_file(): seen.setdefault(top.name, []).append(top.relative_to(repo).as_posix()) + # Hybrid dir: also scan immediate subdirs for nested skills. + # Intentionally scans only 1 level deep (vs. generate-skill-index.py's 2 levels): + # this is a fast-fail gate, not an exhaustive mirror of the indexer. + for child in sorted(top.iterdir()): + if child.is_dir() and (child / "SKILL.md").is_file(): + seen.setdefault(child.name, []).append(child.relative_to(repo).as_posix()) continue for child in sorted(top.iterdir()): if child.is_dir() and (child / "SKILL.md").is_file(): diff --git a/skills/meta/toolkit/references/toolkit-evolution.md b/skills/meta/toolkit/references/toolkit-evolution.md index dfd708b7..c48caac8 100644 --- a/skills/meta/toolkit/references/toolkit-evolution.md +++ b/skills/meta/toolkit/references/toolkit-evolution.md @@ -238,7 +238,7 @@ Win condition for each implementation: **Step 1: Handle winners (WIN status)** -For each winning implementation, create a PR using the template from `references/evolve-scripts.md` § Step 1, then merge. After creating the PR, run pr-review to validate, then merge. +For each winning implementation, create a PR using the template from `references/evolve-scripts.md` § Step 1, then merge. After creating the PR, run /pr-review to validate, then merge. The multi-persona critique + A/B testing gate is the review. Auto-merge is safe because the validation happened before this step. diff --git a/skills/process/pr-workflow/references/fix.md b/skills/process/pr-workflow/references/fix.md index 98164f34..7c524955 100644 --- a/skills/process/pr-workflow/references/fix.md +++ b/skills/process/pr-workflow/references/fix.md @@ -206,8 +206,8 @@ Solution: ### Related Skills - `pr-pipeline` — For creating PRs from scratch -- `pr-review` — For reviewing code without fixing -- `systematic-debugging` — For general debugging unrelated to PR comments +- `review` — For reviewing code without fixing (skill); use `/pr-review` command for full multi-agent PR review +- `debugging` — For general debugging unrelated to PR comments ### PR Comment Best Practices - Always validate before fixing (this prevents introducing bugs that reviewers caught) diff --git a/skills/process/pr-workflow/references/sync.md b/skills/process/pr-workflow/references/sync.md index 95bb8ff2..2af2f3c6 100644 --- a/skills/process/pr-workflow/references/sync.md +++ b/skills/process/pr-workflow/references/sync.md @@ -221,7 +221,7 @@ Solution: ## References -- `/pr-review` -- Comprehensive PR review (used in the review-fix loop) +- `/pr-review` -- Comprehensive PR review slash command (not the `review` skill; used in the review-fix loop) - `/pr-cleanup` -- Post-merge branch cleanup - `scripts/classify-repo.py` -- Repo classification for workflow gating - `scripts/adr-decision-coverage.py` -- ADR decision coverage checker