From b7923c627bd861b9d6e077bc291362ac7d413a90 Mon Sep 17 00:00:00 2001 From: sergio-sisternes-epam Date: Tue, 24 Feb 2026 18:30:41 +0000 Subject: [PATCH 1/2] feat: promote .apm/skills/ sub-skills to top-level .github/skills/ entries Sub-skills placed under .apm/skills//SKILL.md are now promoted to independent top-level entries in .github/skills// during install. This allows Copilot to discover them independently, since it only scans direct children of .github/skills/. Closes #101 --- docs/skills.md | 42 ++++ src/apm_cli/integration/skill_integrator.py | 56 ++++++ .../unit/integration/test_skill_integrator.py | 179 +++++++++++++++++- 3 files changed, 276 insertions(+), 1 deletion(-) diff --git a/docs/skills.md b/docs/skills.md index 4f7786b1..186546b2 100644 --- a/docs/skills.md +++ b/docs/skills.md @@ -43,6 +43,7 @@ APM copies skills directly to `.github/skills/` (primary) and `.claude/skills/` | Package Type | Behavior | |--------------|----------| | **Has existing SKILL.md** | Entire skill folder copied to `.github/skills/{skill-name}/` | +| **Has sub-skills in `.apm/skills/`** | Each `.apm/skills/*/SKILL.md` also promoted to `.github/skills/{sub-skill-name}/` | | **No SKILL.md and no primitives** | No skill folder created | **Target Directories:** @@ -227,6 +228,47 @@ apm install your-org/awesome-skills/skill-1 apm install your-org/awesome-skills/skill-2 ``` +### Option 4: Multi-skill Package + +Bundle multiple skills inside a single APM package using `.apm/skills/`: + +``` +my-package/ +├── apm.yml +├── SKILL.md # Parent skill (package-level guide) +└── .apm/ + ├── instructions/ + ├── prompts/ + └── skills/ + ├── skill-a/ + │ └── SKILL.md # Sub-skill A + └── skill-b/ + └── SKILL.md # Sub-skill B +``` + +On install, APM promotes each sub-skill to a top-level `.github/skills/` entry alongside the parent — see [Sub-skill Promotion](#sub-skill-promotion) below. + +### Sub-skill Promotion + +When a package contains sub-skills in `.apm/skills/*/` subdirectories, APM promotes each to a top-level entry under `.github/skills/`. This ensures Copilot can discover them independently, since it only scans direct children of `.github/skills/`. + +``` +# Installed package with sub-skills: +apm_modules/org/repo/my-package/ +├── SKILL.md +└── .apm/ + └── skills/ + └── azure-naming/ + └── SKILL.md + +# Result after install: +.github/skills/ +├── my-package/ # Parent skill +│ └── SKILL.md +└── azure-naming/ # Promoted sub-skill + └── SKILL.md +``` + ## Package Detection APM automatically detects package types: diff --git a/src/apm_cli/integration/skill_integrator.py b/src/apm_cli/integration/skill_integrator.py index 2636523d..ed270f2f 100644 --- a/src/apm_cli/integration/skill_integrator.py +++ b/src/apm_cli/integration/skill_integrator.py @@ -815,6 +815,35 @@ def _integrate_native_skill( files_copied = sum(1 for _ in github_skill_dir.rglob('*') if _.is_file()) + # === Promote sub-skills to top-level entries === + # Packages may contain sub-skills in .apm/skills/*/ subdirectories. + # Copilot only discovers .github/skills//SKILL.md (direct children), + # so we promote each sub-skill to an independent top-level entry. + github_skills_root = project_root / ".github" / "skills" + sub_skills_dir = package_path / ".apm" / "skills" + if sub_skills_dir.is_dir(): + for sub_skill_path in sub_skills_dir.iterdir(): + if not sub_skill_path.is_dir(): + continue + if not (sub_skill_path / "SKILL.md").exists(): + continue + raw_sub_name = sub_skill_path.name + is_valid, _ = validate_skill_name(raw_sub_name) + sub_name = raw_sub_name if is_valid else normalize_skill_name(raw_sub_name) + target = github_skills_root / sub_name + if target.exists(): + try: + from apm_cli.cli import _rich_warning + _rich_warning( + f"Sub-skill '{sub_name}' from '{skill_name}' overwrites existing skill at .github/skills/{sub_name}/" + ) + except ImportError: + pass + shutil.rmtree(target) + target.mkdir(parents=True, exist_ok=True) + shutil.copytree(sub_skill_path, target, dirs_exist_ok=True) + files_copied += sum(1 for _ in target.rglob('*') if _.is_file()) + # === T7: Copy to .claude/skills/ (secondary - compatibility) === claude_dir = project_root / ".claude" if claude_dir.exists() and claude_dir.is_dir(): @@ -825,6 +854,23 @@ def _integrate_native_skill( claude_skill_dir.parent.mkdir(parents=True, exist_ok=True) shutil.copytree(package_path, claude_skill_dir) + + # Promote sub-skills for Claude too + if sub_skills_dir.is_dir(): + claude_skills_root = claude_dir / "skills" + for sub_skill_path in sub_skills_dir.iterdir(): + if not sub_skill_path.is_dir(): + continue + if not (sub_skill_path / "SKILL.md").exists(): + continue + raw_sub_name = sub_skill_path.name + is_valid, _ = validate_skill_name(raw_sub_name) + sub_name = raw_sub_name if is_valid else normalize_skill_name(raw_sub_name) + target = claude_skills_root / sub_name + if target.exists(): + shutil.rmtree(target) + target.mkdir(parents=True, exist_ok=True) + shutil.copytree(sub_skill_path, target, dirs_exist_ok=True) return SkillIntegrationResult( skill_created=skill_created, @@ -1031,6 +1077,16 @@ def sync_integration(self, apm_package, project_root: Path) -> Dict[str, int]: is_valid, _ = validate_skill_name(raw_name) skill_name = raw_name if is_valid else normalize_skill_name(raw_name) installed_skill_names.add(skill_name) + + # Also include promoted sub-skills from installed packages + install_path = dep.get_install_path(project_root / "apm_modules") + sub_skills_dir = install_path / ".apm" / "skills" + if sub_skills_dir.is_dir(): + for sub_skill_path in sub_skills_dir.iterdir(): + if sub_skill_path.is_dir() and (sub_skill_path / "SKILL.md").exists(): + raw_sub = sub_skill_path.name + is_valid, _ = validate_skill_name(raw_sub) + installed_skill_names.add(raw_sub if is_valid else normalize_skill_name(raw_sub)) # Clean .github/skills/ (primary) github_skills_dir = project_root / ".github" / "skills" diff --git a/tests/unit/integration/test_skill_integrator.py b/tests/unit/integration/test_skill_integrator.py index e2a429a7..713e311e 100644 --- a/tests/unit/integration/test_skill_integrator.py +++ b/tests/unit/integration/test_skill_integrator.py @@ -3,7 +3,7 @@ import tempfile import shutil from pathlib import Path -from unittest.mock import Mock +from unittest.mock import Mock, patch from datetime import datetime from apm_cli.integration.skill_integrator import SkillIntegrator, SkillIntegrationResult, to_hyphen_case, validate_skill_name, normalize_skill_name, copy_skill_to_target @@ -2589,3 +2589,180 @@ def test_sync_aggregates_stats_from_both_locations(self): assert result['files_removed'] == 2 assert not github_orphan.exists() assert not claude_orphan.exists() + + +class TestSubSkillPromotion: + """Test that sub-skills inside packages are promoted to top-level entries. + + When a package contains .apm/skills//SKILL.md, each sub-skill + should be copied to .github/skills// as an independent + top-level entry so Copilot can discover it. + """ + + def setup_method(self): + self.temp_dir = tempfile.mkdtemp() + self.project_root = Path(self.temp_dir) + self.integrator = SkillIntegrator() + + def teardown_method(self): + shutil.rmtree(self.temp_dir, ignore_errors=True) + + def _create_package_info( + self, + name: str = "test-pkg", + install_path: Path = None, + package_type: PackageType = PackageType.CLAUDE_SKILL + ) -> PackageInfo: + package = APMPackage( + name=name, + version="1.0.0", + package_path=install_path or self.project_root / "package", + source=f"github.com/test/{name}" + ) + resolved_ref = ResolvedReference( + original_ref="main", + ref_type=GitReferenceType.BRANCH, + resolved_commit="abc123", + ref_name="main" + ) + return PackageInfo( + package=package, + install_path=install_path or self.project_root / "package", + resolved_reference=resolved_ref, + installed_at=datetime.now().isoformat(), + package_type=package_type + ) + + def _create_package_with_sub_skills(self, name="parent-skill", sub_skills=None): + """Create a package directory with a SKILL.md and sub-skills under .apm/skills/.""" + package_dir = self.project_root / name + package_dir.mkdir() + (package_dir / "SKILL.md").write_text(f"---\nname: {name}\ndescription: Parent skill\n---\n# {name}\n") + if sub_skills: + skills_dir = package_dir / ".apm" / "skills" + skills_dir.mkdir(parents=True) + for sub_name in sub_skills: + sub_dir = skills_dir / sub_name + sub_dir.mkdir() + (sub_dir / "SKILL.md").write_text( + f"---\nname: {sub_name}\ndescription: Sub-skill {sub_name}\n---\n# {sub_name}\n" + ) + return package_dir + + def test_sub_skill_promoted_to_top_level(self): + """Sub-skills under .apm/skills/ should be copied to .github/skills/ as top-level entries.""" + package_dir = self._create_package_with_sub_skills( + "modernisation", sub_skills=["azure-naming"] + ) + pkg_info = self._create_package_info(name="modernisation", install_path=package_dir) + + self.integrator.integrate_package_skill(pkg_info, self.project_root) + + # Parent skill exists + assert (self.project_root / ".github" / "skills" / "modernisation" / "SKILL.md").exists() + # Sub-skill promoted to top level + assert (self.project_root / ".github" / "skills" / "azure-naming" / "SKILL.md").exists() + content = (self.project_root / ".github" / "skills" / "azure-naming" / "SKILL.md").read_text() + assert "azure-naming" in content + + def test_multiple_sub_skills_promoted(self): + """All sub-skills in the package should be promoted.""" + package_dir = self._create_package_with_sub_skills( + "my-package", sub_skills=["skill-a", "skill-b", "skill-c"] + ) + pkg_info = self._create_package_info(name="my-package", install_path=package_dir) + + self.integrator.integrate_package_skill(pkg_info, self.project_root) + + for sub in ["skill-a", "skill-b", "skill-c"]: + assert (self.project_root / ".github" / "skills" / sub / "SKILL.md").exists() + + def test_sub_skill_without_skill_md_not_promoted(self): + """Directories under .apm/skills/ without SKILL.md should be ignored.""" + package_dir = self._create_package_with_sub_skills("pkg", sub_skills=["valid-sub"]) + # Add a directory without SKILL.md + (package_dir / ".apm" / "skills" / "no-skill-md").mkdir() + (package_dir / ".apm" / "skills" / "no-skill-md" / "README.md").write_text("# Not a skill") + + pkg_info = self._create_package_info(name="pkg", install_path=package_dir) + self.integrator.integrate_package_skill(pkg_info, self.project_root) + + assert (self.project_root / ".github" / "skills" / "valid-sub" / "SKILL.md").exists() + assert not (self.project_root / ".github" / "skills" / "no-skill-md").exists() + + def test_sub_skill_name_collision_overwrites_with_warning(self): + """If a promoted sub-skill name clashes with an existing skill, it overwrites it.""" + # Pre-existing skill at top level + existing = self.project_root / ".github" / "skills" / "azure-naming" + existing.mkdir(parents=True) + (existing / "SKILL.md").write_text("# Old content") + + package_dir = self._create_package_with_sub_skills( + "modernisation", sub_skills=["azure-naming"] + ) + pkg_info = self._create_package_info(name="modernisation", install_path=package_dir) + + with patch("apm_cli.cli._rich_warning"): + self.integrator.integrate_package_skill(pkg_info, self.project_root) + + # Should be overwritten with sub-skill content + content = (self.project_root / ".github" / "skills" / "azure-naming" / "SKILL.md").read_text() + assert "Sub-skill azure-naming" in content + assert "Old content" not in content + + def test_sub_skill_promoted_to_claude_skills(self): + """Sub-skills should also be promoted under .claude/skills/ when .claude/ exists.""" + (self.project_root / ".claude").mkdir() + package_dir = self._create_package_with_sub_skills( + "modernisation", sub_skills=["azure-naming"] + ) + pkg_info = self._create_package_info(name="modernisation", install_path=package_dir) + + self.integrator.integrate_package_skill(pkg_info, self.project_root) + + assert (self.project_root / ".github" / "skills" / "azure-naming" / "SKILL.md").exists() + assert (self.project_root / ".claude" / "skills" / "azure-naming" / "SKILL.md").exists() + + def test_package_without_sub_skills_unchanged(self): + """Packages without .apm/skills/ subdirectory should work as before.""" + package_dir = self.project_root / "simple-skill" + package_dir.mkdir() + (package_dir / "SKILL.md").write_text("---\nname: simple-skill\n---\n# Simple") + + pkg_info = self._create_package_info(name="simple-skill", install_path=package_dir) + result = self.integrator.integrate_package_skill(pkg_info, self.project_root) + + assert result.skill_created is True + assert (self.project_root / ".github" / "skills" / "simple-skill" / "SKILL.md").exists() + skills = list((self.project_root / ".github" / "skills").iterdir()) + assert len(skills) == 1 + + def test_sync_integration_preserves_promoted_sub_skills(self): + """sync_integration should not orphan promoted sub-skills.""" + # Set up installed package structure in apm_modules + apm_modules = self.project_root / "apm_modules" + owner_dir = apm_modules / "testorg" / "agent-library" / "modernisation" + owner_dir.mkdir(parents=True) + (owner_dir / "apm.yml").write_text("name: modernisation\nversion: 1.0.0\n") + (owner_dir / "SKILL.md").write_text("---\nname: modernisation\n---\n# Parent") + sub_dir = owner_dir / ".apm" / "skills" / "azure-naming" + sub_dir.mkdir(parents=True) + (sub_dir / "SKILL.md").write_text("---\nname: azure-naming\n---\n# Sub") + + # Create the promoted skills in .github/skills/ + for name in ["modernisation", "azure-naming"]: + d = self.project_root / ".github" / "skills" / name + d.mkdir(parents=True) + (d / "SKILL.md").write_text(f"# {name}") + + # Mock the dependency + dep = DependencyReference.parse("testorg/agent-library/modernisation") + apm_package = Mock() + apm_package.get_apm_dependencies.return_value = [dep] + + result = self.integrator.sync_integration(apm_package, self.project_root) + + # Neither should be removed + assert result['files_removed'] == 0 + assert (self.project_root / ".github" / "skills" / "modernisation").exists() + assert (self.project_root / ".github" / "skills" / "azure-naming").exists() From 252e2443b065fc8f8d25cdd83cfb50fba04186ee Mon Sep 17 00:00:00 2001 From: sergio-sisternes-epam Date: Tue, 24 Feb 2026 19:08:10 +0000 Subject: [PATCH 2/2] refactor: address PR review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Exclude .apm/ from parent copy to avoid redundant storage - Extract _promote_sub_skills() helper to eliminate duplication - Assert warning is emitted on sub-skill name collision - Add test for sub-skill name normalization (invalid → valid) --- src/apm_cli/integration/skill_integrator.py | 82 ++++++++++--------- .../unit/integration/test_skill_integrator.py | 30 ++++++- 2 files changed, 70 insertions(+), 42 deletions(-) diff --git a/src/apm_cli/integration/skill_integrator.py b/src/apm_cli/integration/skill_integrator.py index ed270f2f..56b5df3c 100644 --- a/src/apm_cli/integration/skill_integrator.py +++ b/src/apm_cli/integration/skill_integrator.py @@ -739,6 +739,40 @@ def _generate_skill_file(self, package_info, primitives: dict, skill_dir: Path) return files_copied + @staticmethod + def _promote_sub_skills(sub_skills_dir: Path, target_skills_root: Path, parent_name: str, *, warn: bool = True) -> None: + """Promote sub-skills from .apm/skills/ to top-level skill entries. + + Args: + sub_skills_dir: Path to the .apm/skills/ directory in the source package. + target_skills_root: Root skills directory (e.g. .github/skills/ or .claude/skills/). + parent_name: Name of the parent skill (used in warning messages). + warn: Whether to emit a warning on name collisions. + """ + if not sub_skills_dir.is_dir(): + return + for sub_skill_path in sub_skills_dir.iterdir(): + if not sub_skill_path.is_dir(): + continue + if not (sub_skill_path / "SKILL.md").exists(): + continue + raw_sub_name = sub_skill_path.name + is_valid, _ = validate_skill_name(raw_sub_name) + sub_name = raw_sub_name if is_valid else normalize_skill_name(raw_sub_name) + target = target_skills_root / sub_name + if target.exists(): + if warn: + try: + from apm_cli.cli import _rich_warning + _rich_warning( + f"Sub-skill '{sub_name}' from '{parent_name}' overwrites existing skill at {target_skills_root.name}/{sub_name}/" + ) + except ImportError: + pass + shutil.rmtree(target) + target.mkdir(parents=True, exist_ok=True) + shutil.copytree(sub_skill_path, target, dirs_exist_ok=True) + def _integrate_native_skill( self, package_info, project_root: Path, source_skill_md: Path ) -> SkillIntegrationResult: @@ -811,7 +845,8 @@ def _integrate_native_skill( shutil.rmtree(github_skill_dir) github_skill_dir.parent.mkdir(parents=True, exist_ok=True) - shutil.copytree(package_path, github_skill_dir) + shutil.copytree(package_path, github_skill_dir, + ignore=shutil.ignore_patterns('.apm')) files_copied = sum(1 for _ in github_skill_dir.rglob('*') if _.is_file()) @@ -819,30 +854,9 @@ def _integrate_native_skill( # Packages may contain sub-skills in .apm/skills/*/ subdirectories. # Copilot only discovers .github/skills//SKILL.md (direct children), # so we promote each sub-skill to an independent top-level entry. - github_skills_root = project_root / ".github" / "skills" sub_skills_dir = package_path / ".apm" / "skills" - if sub_skills_dir.is_dir(): - for sub_skill_path in sub_skills_dir.iterdir(): - if not sub_skill_path.is_dir(): - continue - if not (sub_skill_path / "SKILL.md").exists(): - continue - raw_sub_name = sub_skill_path.name - is_valid, _ = validate_skill_name(raw_sub_name) - sub_name = raw_sub_name if is_valid else normalize_skill_name(raw_sub_name) - target = github_skills_root / sub_name - if target.exists(): - try: - from apm_cli.cli import _rich_warning - _rich_warning( - f"Sub-skill '{sub_name}' from '{skill_name}' overwrites existing skill at .github/skills/{sub_name}/" - ) - except ImportError: - pass - shutil.rmtree(target) - target.mkdir(parents=True, exist_ok=True) - shutil.copytree(sub_skill_path, target, dirs_exist_ok=True) - files_copied += sum(1 for _ in target.rglob('*') if _.is_file()) + github_skills_root = project_root / ".github" / "skills" + self._promote_sub_skills(sub_skills_dir, github_skills_root, skill_name, warn=True) # === T7: Copy to .claude/skills/ (secondary - compatibility) === claude_dir = project_root / ".claude" @@ -853,24 +867,12 @@ def _integrate_native_skill( shutil.rmtree(claude_skill_dir) claude_skill_dir.parent.mkdir(parents=True, exist_ok=True) - shutil.copytree(package_path, claude_skill_dir) + shutil.copytree(package_path, claude_skill_dir, + ignore=shutil.ignore_patterns('.apm')) # Promote sub-skills for Claude too - if sub_skills_dir.is_dir(): - claude_skills_root = claude_dir / "skills" - for sub_skill_path in sub_skills_dir.iterdir(): - if not sub_skill_path.is_dir(): - continue - if not (sub_skill_path / "SKILL.md").exists(): - continue - raw_sub_name = sub_skill_path.name - is_valid, _ = validate_skill_name(raw_sub_name) - sub_name = raw_sub_name if is_valid else normalize_skill_name(raw_sub_name) - target = claude_skills_root / sub_name - if target.exists(): - shutil.rmtree(target) - target.mkdir(parents=True, exist_ok=True) - shutil.copytree(sub_skill_path, target, dirs_exist_ok=True) + claude_skills_root = claude_dir / "skills" + self._promote_sub_skills(sub_skills_dir, claude_skills_root, skill_name, warn=False) return SkillIntegrationResult( skill_created=skill_created, diff --git a/tests/unit/integration/test_skill_integrator.py b/tests/unit/integration/test_skill_integrator.py index 713e311e..33273b5d 100644 --- a/tests/unit/integration/test_skill_integrator.py +++ b/tests/unit/integration/test_skill_integrator.py @@ -2660,6 +2660,8 @@ def test_sub_skill_promoted_to_top_level(self): # Parent skill exists assert (self.project_root / ".github" / "skills" / "modernisation" / "SKILL.md").exists() + # .apm/ excluded from parent copy to avoid redundant storage + assert not (self.project_root / ".github" / "skills" / "modernisation" / ".apm").exists() # Sub-skill promoted to top level assert (self.project_root / ".github" / "skills" / "azure-naming" / "SKILL.md").exists() content = (self.project_root / ".github" / "skills" / "azure-naming" / "SKILL.md").read_text() @@ -2691,7 +2693,7 @@ def test_sub_skill_without_skill_md_not_promoted(self): assert not (self.project_root / ".github" / "skills" / "no-skill-md").exists() def test_sub_skill_name_collision_overwrites_with_warning(self): - """If a promoted sub-skill name clashes with an existing skill, it overwrites it.""" + """If a promoted sub-skill name clashes with an existing skill, it overwrites and warns.""" # Pre-existing skill at top level existing = self.project_root / ".github" / "skills" / "azure-naming" existing.mkdir(parents=True) @@ -2702,9 +2704,14 @@ def test_sub_skill_name_collision_overwrites_with_warning(self): ) pkg_info = self._create_package_info(name="modernisation", install_path=package_dir) - with patch("apm_cli.cli._rich_warning"): + with patch("apm_cli.cli._rich_warning") as mock_warning: self.integrator.integrate_package_skill(pkg_info, self.project_root) + # Warning should have been emitted about the collision + mock_warning.assert_called_once() + assert "azure-naming" in mock_warning.call_args[0][0] + assert "modernisation" in mock_warning.call_args[0][0] + # Should be overwritten with sub-skill content content = (self.project_root / ".github" / "skills" / "azure-naming" / "SKILL.md").read_text() assert "Sub-skill azure-naming" in content @@ -2723,6 +2730,25 @@ def test_sub_skill_promoted_to_claude_skills(self): assert (self.project_root / ".github" / "skills" / "azure-naming" / "SKILL.md").exists() assert (self.project_root / ".claude" / "skills" / "azure-naming" / "SKILL.md").exists() + def test_sub_skill_name_normalization(self): + """Sub-skills with invalid names should be normalized before promotion.""" + package_dir = self.project_root / "my-package" + package_dir.mkdir() + (package_dir / "SKILL.md").write_text("---\nname: my-package\n---\n# Parent") + skills_dir = package_dir / ".apm" / "skills" + skills_dir.mkdir(parents=True) + # Create sub-skill with invalid name (uppercase + underscores) + bad_name_dir = skills_dir / "My_Azure_Skill" + bad_name_dir.mkdir() + (bad_name_dir / "SKILL.md").write_text("---\nname: My_Azure_Skill\n---\n# Bad name") + + pkg_info = self._create_package_info(name="my-package", install_path=package_dir) + self.integrator.integrate_package_skill(pkg_info, self.project_root) + + # Should be normalized to lowercase-hyphenated + assert not (self.project_root / ".github" / "skills" / "My_Azure_Skill").exists() + assert (self.project_root / ".github" / "skills" / "my-azure-skill" / "SKILL.md").exists() + def test_package_without_sub_skills_unchanged(self): """Packages without .apm/skills/ subdirectory should work as before.""" package_dir = self.project_root / "simple-skill"