diff --git a/.gitignore b/.gitignore index ed4e15048..553434393 100644 --- a/.gitignore +++ b/.gitignore @@ -61,4 +61,7 @@ docs/wip/ .env.*.local AGENTS.md PRD.md -PRD*.md \ No newline at end of file +PRD*.md +# Internal planning documents +skill-plan.md +skill-strategy.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 8cb37b142..e8c3e0dac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,20 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.7.0] - 2025-12-19 + +### Changed + +- **Native Skills Support**: Skills now install to `.github/skills/` as the primary target (per [agentskills.io](https://agentskills.io/) standard) +- **Skills ≠ Agents**: Removed skill → agent transformation; skills and agents are now separate primitives +- **Explicit Package Types**: Added `type` field to apm.yml (`instructions`, `skill`, `hybrid`, `prompts`) for routing control +- **Skill Name Validation**: Validates and normalizes skill names per agentskills.io spec (lowercase, hyphens, 1-64 chars) +- **Claude Compatibility**: Skills also copy to `.claude/skills/` when `.claude/` folder exists + +### Added + +- Auto-creates `.github/` directory on install if neither `.github/` nor `.claude/` exists + ## [0.6.3] - 2025-12-09 ### Fixed diff --git a/docs/cli-reference.md b/docs/cli-reference.md index 7c9668a63..d031f97cd 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -224,8 +224,13 @@ When you run `apm install`, APM automatically integrates primitives from install APM also integrates with Claude Code when `.claude/` directory exists: - **Commands**: `.prompt.md` files → `.claude/commands/*-apm.md` -- **Skills**: Skill packages → `.claude/skills/{folder-name}/` (copies entire folder) -- **APM packages**: Packages with `.apm/` primitives → `.claude/skills/{folder-name}/SKILL.md` (generated) + +**Skill Integration:** + +Skills are copied directly to target directories: + +- **Primary**: `.github/skills/{skill-name}/` — Entire skill folder copied +- **Compatibility**: `.claude/skills/{skill-name}/` — Also copied if `.claude/` folder exists **Example Integration Output**: ``` @@ -270,7 +275,7 @@ apm uninstall danielmeppiel/design-guidelines --dry-run | Integrated agents | `.github/agents/*-apm.agent.md` | | Integrated chatmodes | `.github/agents/*-apm.chatmode.md` | | Claude commands | `.claude/commands/*-apm.md` | -| Skill folders | `.claude/skills/{folder-name}/` | +| Skill folders | `.github/skills/{folder-name}/` | **Behavior:** - Removes package from `apm.yml` dependencies @@ -632,8 +637,8 @@ target: vscode # or claude, or all | Target | Output Files | Best For | |--------|--------------|----------| -| `vscode` | AGENTS.md, .github/prompts/, .github/agents/ | GitHub Copilot, Cursor, Codex, Gemini | -| `claude` | CLAUDE.md, .claude/commands/, .claude/skills/, SKILL.md | Claude Code, Claude Desktop | +| `vscode` | AGENTS.md, .github/prompts/, .github/agents/, .github/skills/ | GitHub Copilot, Cursor, Codex, Gemini | +| `claude` | CLAUDE.md, .claude/commands/, SKILL.md | Claude Code, Claude Desktop | | `all` | All of the above | Universal compatibility | **Examples:** diff --git a/docs/compilation.md b/docs/compilation.md index 4655dd1bf..cfff5149b 100644 --- a/docs/compilation.md +++ b/docs/compilation.md @@ -41,7 +41,7 @@ target: vscode # or claude, or all | `all` | Both `AGENTS.md` and `CLAUDE.md` | Universal compatibility | | `minimal` | `AGENTS.md` only | Works everywhere, no folder integration | -> **Note**: `AGENTS.md` and `CLAUDE.md` contain **only instructions** (grouped by `applyTo` patterns). Prompts, agents, commands, and skills are integrated by `apm install`, not `apm compile`. See the [Integrations Guide](integrations.md) for details on how `apm install` populates `.github/prompts/`, `.github/agents/`, `.claude/commands/`, and `.claude/skills/`. +> **Note**: `AGENTS.md` and `CLAUDE.md` contain **only instructions** (grouped by `applyTo` patterns). Prompts, agents, commands, and skills are integrated by `apm install`, not `apm compile`. See the [Integrations Guide](integrations.md) for details on how `apm install` populates `.github/prompts/`, `.github/agents/`, `.github/skills/`, and `.claude/commands/`. ### How It Works diff --git a/docs/dependencies.md b/docs/dependencies.md index 78a574d3c..3a2cd4bcf 100644 --- a/docs/dependencies.md +++ b/docs/dependencies.md @@ -41,12 +41,12 @@ apm install ComposioHQ/awesome-claude-skills/brand-guidelines #### Skill Integration During Install -When your project has a `.claude/` folder, skills are integrated automatically: +Skills are integrated to `.github/skills/`: | Source | Result | |--------|--------| -| Package with existing `SKILL.md` | Skill folder copied to `.claude/skills/{folder-name}/` | -| APM package with `.apm/` primitives (no SKILL.md) | SKILL.md auto-generated, folder copied to `.claude/skills/{folder-name}/` | +| Package with existing `SKILL.md` | Skill folder copied to `.github/skills/{folder-name}/` | +| APM package with `.apm/` primitives (no SKILL.md) | SKILL.md auto-generated, folder copied to `.github/skills/{folder-name}/` | | Package without SKILL.md or primitives | No skill folder created | #### Skill Folder Naming @@ -54,7 +54,7 @@ When your project has a `.claude/` folder, skills are integrated automatically: Skill folders use the **source folder name directly** (not flattened paths): ``` -.claude/skills/ +.github/skills/ ├── brand-guidelines/ # From ComposioHQ/awesome-claude-skills/brand-guidelines ├── mcp-builder/ # From ComposioHQ/awesome-claude-skills/mcp-builder └── compliance-rules/ # From danielmeppiel/compliance-rules diff --git a/docs/getting-started.md b/docs/getting-started.md index 3ccffebbf..2f31fac51 100644 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -389,10 +389,10 @@ apm install For VSCode/Copilot (when `.github/` exists): - `.github/prompts/*-apm.prompt.md` - Reusable prompt templates - `.github/agents/*-apm.agent.md` - Agent definitions +- `.github/skills/{folder-name}/` - Skills with `SKILL.md` meta-guide For Claude Code (when `.claude/` exists): - `.claude/commands/*-apm.md` - Slash commands -- `.claude/skills/{folder-name}/` - Skills with `SKILL.md` meta-guide > **Tip:** Both integrations can coexist in the same project. APM installs to all detected targets. @@ -665,8 +665,8 @@ apm deps list # 🔗 Show installed APM dependencies - `SKILL.md` - Package meta-guide for AI discovery - `AGENTS.md` - Generated VSCode/Copilot instructions - `CLAUDE.md` - Generated Claude Code instructions -- `.github/prompts/`, `.github/agents/` - Installed VSCode primitives -- `.claude/commands/`, `.claude/skills/` - Installed Claude primitives +- `.github/prompts/`, `.github/agents/`, `.github/skills/` - Installed VSCode primitives and skills +- `.claude/commands/` - Installed Claude commands - `apm_modules/` - Installed APM dependencies - `*.prompt.md` - Executable agent workflows diff --git a/docs/integrations.md b/docs/integrations.md index ee18c36bf..a4a1deb04 100644 --- a/docs/integrations.md +++ b/docs/integrations.md @@ -226,7 +226,7 @@ When you run `apm install`, APM integrates package primitives into Claude's nati | Location | Purpose | |----------|---------|| | `.claude/commands/*.md` | Slash commands from installed packages (from `.prompt.md` files) | -| `.claude/skills/{folder}/` | Skills from packages with `SKILL.md` or `.apm/` primitives | +| `.github/skills/{folder}/` | Skills from packages with `SKILL.md` or `.apm/` primitives | ### Automatic Command Integration @@ -250,23 +250,23 @@ apm install danielmeppiel/design-guidelines ### Automatic Skills Integration -APM automatically integrates skills from installed packages into `.claude/skills/`: +APM automatically integrates skills from installed packages into `.github/skills/`: ```bash # Install a package with skills apm install ComposioHQ/awesome-claude-skills/mcp-builder # Result: -# .claude/skills/mcp-builder/SKILL.md → Skill available in Claude -# .claude/skills/mcp-builder/... → Full skill folder copied +# .github/skills/mcp-builder/SKILL.md → Skill available for agents +# .github/skills/mcp-builder/... → Full skill folder copied ``` **Skill Folder Naming**: Uses the source folder name directly (e.g., `mcp-builder`, `design-guidelines`), not flattened paths. **How skill integration works:** 1. `apm install` checks if the package contains a `SKILL.md` file -2. If `SKILL.md` exists: copies the entire skill folder to `.claude/skills/{folder-name}/` -3. If no `SKILL.md` but package has `.apm/` primitives: auto-generates `SKILL.md` in `.claude/skills/{folder-name}/` +2. If `SKILL.md` exists: copies the entire skill folder to `.github/skills/{folder-name}/` +3. If no `SKILL.md` but package has `.apm/` primitives: auto-generates `SKILL.md` in `.github/skills/{folder-name}/` 4. Updates `.gitignore` to exclude generated skills 5. `apm uninstall` removes the skill folder @@ -318,12 +318,12 @@ apm compile --target claude # /gdpr-assessment → Runs GDPR compliance check # 4. CLAUDE.md provides project instructions automatically -# 5. Skills in .claude/skills/ are available for Claude to reference +# 5. Skills in .github/skills/ are available for agents to reference ``` ### Claude Desktop Integration -Skills installed to `.claude/skills/` are automatically available in Claude Desktop. Each skill folder contains a `SKILL.md` that defines the skill's capabilities and any supporting files. +Skills installed to `.github/skills/` are automatically available for AI agents. Each skill folder contains a `SKILL.md` that defines the skill's capabilities and any supporting files. ### Cleanup and Sync diff --git a/docs/skills.md b/docs/skills.md index 3f2a9ccb7..fd0ecae28 100644 --- a/docs/skills.md +++ b/docs/skills.md @@ -37,21 +37,28 @@ When you run `apm install`, APM handles skill integration automatically: ### Step 1: Download to apm_modules/ APM downloads packages to `apm_modules/owner/repo/` (or `apm_modules/owner/repo/skill-name/` for subdirectory packages). -### Step 2: Skill Integration (if `.claude/` exists) -When your project has a `.claude/` folder, APM integrates skills: +### Step 2: Skill Integration +APM copies skills directly to `.github/skills/` (primary) and `.claude/skills/` (compatibility): | Package Type | Behavior | |--------------|----------| -| **Has existing SKILL.md** | Entire skill folder copied to `.claude/skills/{folder-name}/` | -| **Has `.apm/` primitives but no SKILL.md** | SKILL.md auto-generated, folder copied to `.claude/skills/{folder-name}/` | +| **Has existing SKILL.md** | Entire skill folder copied to `.github/skills/{skill-name}/` | | **No SKILL.md and no primitives** | No skill folder created | +**Target Directories:** +- **Primary**: `.github/skills/{skill-name}/` — Works with Copilot, Cursor, Codex, Gemini +- **Compatibility**: `.claude/skills/{skill-name}/` — Only if `.claude/` folder already exists + ### Skill Folder Naming -Skills use the **source folder name directly**: +Skill names are validated per the [agentskills.io](https://agentskills.io/) spec: +- 1-64 characters +- Lowercase alphanumeric + hyphens only +- No consecutive hyphens (`--`) +- Cannot start/end with hyphen ``` -.claude/skills/ +.github/skills/ ├── mcp-builder/ # From ComposioHQ/awesome-claude-skills/mcp-builder ├── design-guidelines/ # From danielmeppiel/design-guidelines └── compliance-rules/ # From danielmeppiel/compliance-rules @@ -231,14 +238,13 @@ APM automatically detects package types: ## Target Detection -APM decides where to output based on project structure: +APM decides where to output skills based on project structure: -| Condition | Target | Skill Output | -|-----------|--------|--------------| -| `.github/` exists | VSCode | `.github/agents/*.agent.md` | -| `.claude/` exists | Claude | Native SKILL.md | -| Both exist | All | Both outputs | -| Neither | Minimal | AGENTS.md only | +| Condition | Skill Output | +|-----------|---------------| +| `.github/` exists | `.github/skills/{skill-name}/SKILL.md` | +| `.claude/` also exists | Also copies to `.claude/skills/{skill-name}/SKILL.md` | +| Neither exists | Creates `.github/skills/` | Override with: ```bash @@ -320,16 +326,16 @@ Error: Could not find SKILL.md or apm.yml apm install owner/repo/subdirectory ``` -### Agent Not Generated +### Skill Name Validation Error -If `.github/agents/*.agent.md` isn't created: +If you see a skill name validation warning: -1. **Check target:** Create `.github/` folder or use `--target vscode` -2. **Reinstall:** Run `apm install package-name` again +1. **Check naming:** Names must be lowercase, 1-64 chars, hyphens only (no underscores) +2. **Auto-normalization:** APM automatically normalizes invalid names when possible ### Metadata Missing -If agent lacks APM metadata: +If skill lacks APM metadata: 1. Check the skill was installed via APM (not manually copied) 2. Reinstall the package diff --git a/pyproject.toml b/pyproject.toml index fe5d5b50e..a5ab856b9 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "apm-cli" -version = "0.6.3" +version = "0.7.0" description = "MCP configuration tool" readme = "README.md" requires-python = ">=3.9" diff --git a/src/apm_cli/cli.py b/src/apm_cli/cli.py index fd30d5c37..89d64c0ee 100644 --- a/src/apm_cli/cli.py +++ b/src/apm_cli/cli.py @@ -1190,10 +1190,11 @@ def uninstall(ctx, packages, dry_run): except Exception as e: agents_failed += 1 - # Sync skill integration to remove orphaned Claude skills + # Sync skill integration to remove orphaned skills + # T12: Check both .github/skills/ and .claude/skills/ locations skills_cleaned = 0 skills_failed = 0 - if Path(".claude/skills").exists(): + if Path(".github/skills").exists() or Path(".claude/skills").exists(): try: from apm_cli.models.apm_package import APMPackage from apm_cli.integration.skill_integrator import SkillIntegrator @@ -1228,9 +1229,9 @@ def uninstall(ctx, packages, dry_run): if agents_cleaned > 0: _rich_info(f"✓ Cleaned up {agents_cleaned} integrated agent(s)") if skills_cleaned > 0: - _rich_info(f"✓ Cleaned up {skills_cleaned} Claude skill(s)") + _rich_info(f"✓ Cleaned up {skills_cleaned} skill(s)") if commands_cleaned > 0: - _rich_info(f"✓ Cleaned up {commands_cleaned} Claude command(s)") + _rich_info(f"✓ Cleaned up {commands_cleaned} command(s)") if ( prompts_failed > 0 or agents_failed > 0 @@ -1355,6 +1356,19 @@ def matches_filter(dep): # Get config target from apm.yml if available config_target = apm_package.target + # Auto-create .github/ if neither .github/ nor .claude/ exists. + # Per skill-strategy Decision 1, .github/skills/ is the standard skills location; + # creating .github/ here ensures a consistent skills root and also enables + # VSCode/Copilot integration by default (quick path to value), even for + # projects that don't yet use .claude/. + github_dir = project_root / ".github" + claude_dir = project_root / ".claude" + if not github_dir.exists() and not claude_dir.exists(): + github_dir.mkdir(parents=True, exist_ok=True) + _rich_info( + "Created .github/ as standard skills root (.github/skills/) and to enable VSCode/Copilot integration" + ) + detected_target, detection_reason = detect_target( project_root=project_root, explicit_target=None, # No explicit flag for install @@ -1368,7 +1382,7 @@ def matches_filter(dep): # Initialize integrators prompt_integrator = PromptIntegrator() agent_integrator = AgentIntegrator() - from apm_cli.integration.skill_integrator import SkillIntegrator + from apm_cli.integration.skill_integrator import SkillIntegrator, should_install_skill from apm_cli.integration.command_integrator import CommandIntegrator skill_integrator = SkillIntegrator() @@ -1519,10 +1533,6 @@ def matches_filter(dep): _rich_info( f" └─ {agent_result.files_integrated} agents integrated → .github/agents/" ) - if agent_result.skills_integrated > 0: - _rich_info( - f" └─ {agent_result.skills_integrated} skill(s) transformed → .github/agents/ (from SKILL.md)" - ) if agent_result.files_updated > 0: _rich_info( f" └─ {agent_result.files_updated} agents updated" @@ -1530,19 +1540,20 @@ def matches_filter(dep): # Track links resolved total_links_resolved += agent_result.links_resolved - # Claude integration (skills + commands) - if integrate_claude: - # Generate SKILL.md for Claude Skills + # Skill integration (works for both VSCode and Claude) + # Skills go to .github/skills/ (primary) and .claude/skills/ (if .claude/ exists) + if integrate_vscode or integrate_claude: skill_result = skill_integrator.integrate_package_skill( cached_package_info, project_root ) if skill_result.skill_created: total_skills_generated += 1 _rich_info( - f" └─ SKILL.md generated for Claude Skills" + f" └─ Skill integrated → .github/skills/" ) - # Generate Claude commands from prompts + # Claude-specific integration (commands) + if integrate_claude: command_result = ( command_integrator.integrate_package_commands( cached_package_info, project_root @@ -1607,7 +1618,7 @@ def matches_filter(dep): package_type = package_info.package_type if package_type == PackageType.CLAUDE_SKILL: _rich_info( - f" └─ Package type: Claude Skill (SKILL.md detected)" + f" └─ Package type: Skill (SKILL.md detected)" ) elif package_type == PackageType.HYBRID: _rich_info( @@ -1654,10 +1665,6 @@ def matches_filter(dep): _rich_info( f" └─ {agent_result.files_integrated} agents integrated → .github/agents/" ) - if agent_result.skills_integrated > 0: - _rich_info( - f" └─ {agent_result.skills_integrated} skill(s) transformed → .github/agents/ (from SKILL.md)" - ) if agent_result.files_updated > 0: _rich_info( f" └─ {agent_result.files_updated} agents updated" @@ -1665,18 +1672,20 @@ def matches_filter(dep): # Track links resolved total_links_resolved += agent_result.links_resolved - # Claude integration (skills + commands) - if integrate_claude: - # Generate SKILL.md for Claude Skills + # Skill integration (works for both VSCode and Claude) + # Skills go to .github/skills/ (primary) and .claude/skills/ (if .claude/ exists) + if integrate_vscode or integrate_claude: skill_result = skill_integrator.integrate_package_skill( package_info, project_root ) if skill_result.skill_created: total_skills_generated += 1 _rich_info( - f" └─ SKILL.md generated for Claude Skills" + f" └─ Skill integrated → .github/skills/" ) + # Claude-specific integration (commands) + if integrate_claude: # Generate Claude commands from prompts command_result = ( command_integrator.integrate_package_commands( @@ -1745,11 +1754,11 @@ def matches_filter(dep): # Show Claude Skills stats if any were generated if total_skills_generated > 0: - _rich_info(f"✓ Generated {total_skills_generated} Claude Skill(s)") + _rich_info(f"✓ Generated {total_skills_generated} Skill(s)") # Show Claude commands stats if any were integrated if total_commands_integrated > 0: - _rich_info(f"✓ Integrated {total_commands_integrated} Claude command(s)") + _rich_info(f"✓ Integrated {total_commands_integrated} command(s)") _rich_success(f"Installed {installed_count} APM dependencies") diff --git a/src/apm_cli/integration/__init__.py b/src/apm_cli/integration/__init__.py index 6210c1872..68aa8e295 100644 --- a/src/apm_cli/integration/__init__.py +++ b/src/apm_cli/integration/__init__.py @@ -2,7 +2,28 @@ from .prompt_integrator import PromptIntegrator from .agent_integrator import AgentIntegrator -from .skill_integrator import SkillIntegrator +from .skill_integrator import ( + SkillIntegrator, + validate_skill_name, + normalize_skill_name, + to_hyphen_case, + copy_skill_to_target, + should_install_skill, + should_compile_instructions, + get_effective_type, +) from .skill_transformer import SkillTransformer -__all__ = ['PromptIntegrator', 'AgentIntegrator', 'SkillIntegrator', 'SkillTransformer'] +__all__ = [ + 'PromptIntegrator', + 'AgentIntegrator', + 'SkillIntegrator', + 'SkillTransformer', + 'validate_skill_name', + 'normalize_skill_name', + 'to_hyphen_case', + 'copy_skill_to_target', + 'should_install_skill', + 'should_compile_instructions', + 'get_effective_type', +] diff --git a/src/apm_cli/integration/agent_integrator.py b/src/apm_cli/integration/agent_integrator.py index f71e02368..7ef1ff92f 100644 --- a/src/apm_cli/integration/agent_integrator.py +++ b/src/apm_cli/integration/agent_integrator.py @@ -1,29 +1,36 @@ -"""Agent integration functionality for APM packages.""" +"""Agent integration functionality for APM packages. + +Note: SKILL.md files are NOT transformed to .agent.md files. Skills are handled +separately by SkillIntegrator and installed to .github/skills/ as native skills. +See skill-strategy.md for the full architectural rationale (T5). +""" from pathlib import Path -from typing import List, Dict, Optional, Tuple +from typing import List, Dict from dataclasses import dataclass from datetime import datetime import hashlib import re from .utils import normalize_repo_url -from .skill_transformer import SkillTransformer, to_hyphen_case from apm_cli.compilation.link_resolver import UnifiedLinkResolver from apm_cli.primitives.discovery import discover_primitives -from apm_cli.primitives.parser import parse_skill_file @dataclass class IntegrationResult: - """Result of agent integration operation.""" + """Result of agent integration operation. + + Note: Skills are NOT transformed to agents. They are handled separately + by SkillIntegrator and installed to .github/skills/ as native skills. + See skill-strategy.md for architectural rationale. + """ files_integrated: int files_updated: int # Updated due to version/commit change files_skipped: int # Unchanged (same version/commit) target_paths: List[Path] gitignore_updated: bool links_resolved: int = 0 # Number of context links resolved - skills_integrated: int = 0 # Number of skills transformed to agents class AgentIntegrator: @@ -77,104 +84,13 @@ def find_agent_files(self, package_path: Path) -> List[Path]: return agent_files - def find_skill_file(self, package_path: Path) -> Optional[Path]: - """Find SKILL.md file in a package (Claude Skill format). - - SKILL.md must be at the package root (Claude convention). - - Args: - package_path: Path to the package directory - - Returns: - Optional[Path]: Path to SKILL.md if found, None otherwise - """ - skill_file = package_path / "SKILL.md" - return skill_file if skill_file.exists() else None - - def integrate_skill(self, package_info, project_root: Path) -> Tuple[int, List[Path]]: - """Integrate a SKILL.md from a package into .github/agents/. - - Transforms SKILL.md → .github/agents/{name}.agent.md - - Args: - package_info: PackageInfo object with package metadata - project_root: Root directory of the project - - Returns: - Tuple[int, List[Path]]: (number of skills integrated, list of target paths) - """ - skill_file = self.find_skill_file(package_info.install_path) - if not skill_file: - return (0, []) - - # Parse the skill file - skill = parse_skill_file(skill_file) - if not skill: - return (0, []) - - # Add source attribution - skill.source = f"dependency:{package_info.get_canonical_dependency_string()}" - - # Determine target path - agent_name = to_hyphen_case(skill.name) - agents_dir = project_root / ".github" / "agents" - agents_dir.mkdir(parents=True, exist_ok=True) - target_path = agents_dir / f"{agent_name}.agent.md" - - # Generate content with APM metadata - agent_content = self._generate_skill_agent_content(skill, package_info) - - # Write file - target_path.write_text(agent_content, encoding='utf-8') - - return (1, [target_path]) - - def _generate_skill_agent_content(self, skill, package_info) -> str: - """Generate agent.md content from a skill with APM metadata. - - Args: - skill: Skill primitive to convert - package_info: PackageInfo for source attribution - - Returns: - str: Agent.md file content with frontmatter - """ - import hashlib - from datetime import datetime - - # Calculate content hash - content_hash = hashlib.sha256(skill.content.encode()).hexdigest() - - lines = [ - "---", - f"name: {skill.name}", - ] - - if skill.description: - lines.append(f"description: {skill.description}") - - # Add APM metadata - lines.append("apm:") - lines.append(f" source: {package_info.package.name}") - lines.append(f" source_repo: {package_info.package.source or 'unknown'}") - lines.append(f" source_dependency: {package_info.get_canonical_dependency_string()}") - lines.append(f" version: {package_info.package.version}") - commit = ( - package_info.resolved_reference.resolved_commit - if package_info.resolved_reference - else "unknown" - ) - lines.append(f" commit: {commit}") - lines.append(f" original_path: SKILL.md") - lines.append(f" installed_at: {datetime.now().isoformat()}") - lines.append(f" content_hash: {content_hash}") - lines.append(f" source_type: claude-skill") - - lines.append("---") - lines.append("") - lines.append(skill.content) - - return "\n".join(lines) + # NOTE: find_skill_file(), integrate_skill(), and _generate_skill_agent_content() + # have been REMOVED as part of T5 (skill-strategy.md). + # + # Skills are NOT transformed to .agent.md files. Instead: + # - Skills go directly to .github/skills/ via SkillIntegrator + # - This preserves the native skill format and avoids semantic confusion + # - See skill-strategy.md for the full architectural rationale def _parse_header_metadata(self, file_path: Path) -> dict: """Parse APM metadata from YAML frontmatter in an integrated agent file. @@ -391,6 +307,10 @@ def integrate_package_agents(self, package_info, project_root: Path) -> Integrat - Skip if unchanged (preserve file timestamps) Resolves context links during integration. + Note: SKILL.md files are NOT transformed to .agent.md files. + Skills are handled separately by SkillIntegrator and go to .github/skills/. + See skill-strategy.md for architectural rationale. + Args: package_info: PackageInfo object with package metadata project_root: Root directory of the project @@ -407,14 +327,12 @@ def integrate_package_agents(self, package_info, project_root: Path) -> Integrat # If context discovery fails, continue without link resolution self.link_resolver = None - # Find all agent files in the package + # Find all agent files in the package (.agent.md and .chatmode.md) + # NOTE: SKILL.md is NOT included - skills go to .github/skills/ via SkillIntegrator agent_files = self.find_agent_files(package_info.install_path) - # Also check for SKILL.md (Claude Skill format) - skill_file = self.find_skill_file(package_info.install_path) - - # If no agent files AND no skill file, return empty result - if not agent_files and not skill_file: + # If no agent files, return empty result + if not agent_files: return IntegrationResult( files_integrated=0, files_updated=0, @@ -472,20 +390,13 @@ def integrate_package_agents(self, package_info, project_root: Path) -> Integrat files_integrated += 1 target_paths.append(target_path) - # Also integrate SKILL.md if present (Claude Skill → VSCode agent) - skills_integrated, skill_paths = self.integrate_skill(package_info, project_root) - if skills_integrated > 0: - files_integrated += skills_integrated - target_paths.extend(skill_paths) - return IntegrationResult( files_integrated=files_integrated, files_updated=files_updated, files_skipped=files_skipped, target_paths=target_paths, gitignore_updated=False, - links_resolved=total_links_resolved, - skills_integrated=skills_integrated + links_resolved=total_links_resolved ) def sync_integration(self, apm_package, project_root: Path) -> Dict[str, int]: diff --git a/src/apm_cli/integration/skill_integrator.py b/src/apm_cli/integration/skill_integrator.py index e166280c9..b62efb051 100644 --- a/src/apm_cli/integration/skill_integrator.py +++ b/src/apm_cli/integration/skill_integrator.py @@ -54,6 +54,319 @@ def to_hyphen_case(name: str) -> str: return result[:64] +def validate_skill_name(name: str) -> tuple[bool, str]: + """Validate skill name per agentskills.io spec. + + Skill names must: + - Be 1-64 characters long + - Contain only lowercase alphanumeric characters and hyphens (a-z, 0-9, -) + - Not contain consecutive hyphens (--) + - Not start or end with a hyphen + + Args: + name: Skill name to validate + + Returns: + tuple[bool, str]: (is_valid, error_message) + - is_valid: True if name is valid, False otherwise + - error_message: Empty string if valid, descriptive error otherwise + """ + # Check length + if len(name) < 1: + return (False, "Skill name cannot be empty") + + if len(name) > 64: + return (False, f"Skill name must be 1-64 characters (got {len(name)})") + + # Check for consecutive hyphens + if '--' in name: + return (False, "Skill name cannot contain consecutive hyphens (--)") + + # Check for leading/trailing hyphens + if name.startswith('-'): + return (False, "Skill name cannot start with a hyphen") + + if name.endswith('-'): + return (False, "Skill name cannot end with a hyphen") + + # Check for valid characters (lowercase alphanumeric + hyphens only) + # Pattern: must start and end with alphanumeric, with alphanumeric or hyphens in between + pattern = r'^[a-z0-9]([a-z0-9-]*[a-z0-9])?$' + if not re.match(pattern, name): + # Determine specific error + if any(c.isupper() for c in name): + return (False, "Skill name must be lowercase (no uppercase letters)") + + if '_' in name: + return (False, "Skill name cannot contain underscores (use hyphens instead)") + + if ' ' in name: + return (False, "Skill name cannot contain spaces (use hyphens instead)") + + # Check for other invalid characters + invalid_chars = set(re.findall(r'[^a-z0-9-]', name)) + if invalid_chars: + return (False, f"Skill name contains invalid characters: {', '.join(sorted(invalid_chars))}") + + return (False, "Skill name must be lowercase alphanumeric with hyphens only") + + return (True, "") + + +def normalize_skill_name(name: str) -> str: + """Convert any package name to a valid skill name per agentskills.io spec. + + Normalization steps: + 1. Extract repo name if owner/repo format + 2. Convert to lowercase + 3. Replace underscores and spaces with hyphens + 4. Convert camelCase to hyphen-case + 5. Remove invalid characters + 6. Remove consecutive hyphens + 7. Strip leading/trailing hyphens + 8. Truncate to 64 characters + + Args: + name: Package name to normalize (e.g., "owner/MyRepo_Name") + + Returns: + str: Valid skill name (e.g., "my-repo-name") + """ + # Use to_hyphen_case which already handles most normalization + return to_hyphen_case(name) + + +# ============================================================================= +# Package Type Routing Functions (T4) +# ============================================================================= +# These functions determine behavior based on: +# 1. Explicit `type` field in apm.yml (highest priority) +# 2. Presence of SKILL.md at package root (makes it a skill) +# 3. Default to INSTRUCTIONS for instruction-only packages +# +# Per skill-strategy.md Decision 2: "Skills are explicit, not implicit" +# - Packages with SKILL.md OR explicit type: skill/hybrid → become skills +# - Packages with only instructions → compile to AGENTS.md, NOT skills + +def get_effective_type(package_info) -> "PackageContentType": + """Get effective package content type based on explicit type or package structure. + + Priority order: + 1. Explicit `type` field in apm.yml → use it directly + 2. Package has SKILL.md (PackageType.CLAUDE_SKILL or HYBRID) → treat as SKILL + 3. Otherwise → INSTRUCTIONS (compile to AGENTS.md only) + + Args: + package_info: PackageInfo object containing package metadata + + Returns: + PackageContentType: The effective type + """ + from apm_cli.models.apm_package import PackageContentType, PackageType + + # Priority 1: Explicit type field in apm.yml + pkg_type = package_info.package.type if package_info.package else None + if pkg_type is not None: + return pkg_type + + # Priority 2: Check if package has SKILL.md (via package_type field) + # PackageType.CLAUDE_SKILL = has SKILL.md only + # PackageType.HYBRID = has both apm.yml AND SKILL.md + if package_info.package_type in (PackageType.CLAUDE_SKILL, PackageType.HYBRID): + return PackageContentType.SKILL + + # Priority 3: Default to INSTRUCTIONS for packages without SKILL.md + # Per skill-strategy.md: "Only .instructions.md files → NO skill, compile to AGENTS.md" + return PackageContentType.INSTRUCTIONS + + +def should_install_skill(package_info) -> bool: + """Determine if package should be installed as a native skill. + + This controls whether a package gets installed to .github/skills/ (or .claude/skills/). + + Per skill-strategy.md Decision 2 - "Skills are explicit, not implicit": + + Returns True for: + - SKILL: Package has SKILL.md or declares type: skill + - HYBRID: Package declares type: hybrid in apm.yml + + Returns False for: + - INSTRUCTIONS: Compile to AGENTS.md only, no skill created + - PROMPTS: Commands/prompts only, no skill created + - Packages without SKILL.md and no explicit type field + + Args: + package_info: PackageInfo object containing package metadata + + Returns: + bool: True if package should be installed as a native skill + """ + from apm_cli.models.apm_package import PackageContentType + + effective_type = get_effective_type(package_info) + + # SKILL and HYBRID should install as skills + # INSTRUCTIONS and PROMPTS should NOT install as skills + return effective_type in (PackageContentType.SKILL, PackageContentType.HYBRID) + + +def should_compile_instructions(package_info) -> bool: + """Determine if package should compile to AGENTS.md/CLAUDE.md. + + This controls whether a package's instructions are included in compiled output. + + Per skill-strategy.md Decision 2: + + Returns True for: + - INSTRUCTIONS: Compile to AGENTS.md only (default for packages without SKILL.md) + - HYBRID: Package declares type: hybrid in apm.yml + + Returns False for: + - SKILL: Install as native skill only, no AGENTS.md compilation + - PROMPTS: Commands/prompts only, no instructions compiled + + Args: + package_info: PackageInfo object containing package metadata + + Returns: + bool: True if package's instructions should be compiled to AGENTS.md/CLAUDE.md + """ + from apm_cli.models.apm_package import PackageContentType + + effective_type = get_effective_type(package_info) + + # INSTRUCTIONS and HYBRID should compile to AGENTS.md + # SKILL and PROMPTS should NOT compile to AGENTS.md + return effective_type in (PackageContentType.INSTRUCTIONS, PackageContentType.HYBRID) + + +def copy_skill_to_target( + package_info, + source_path: Path, + target_base: Path, +) -> tuple[Path | None, Path | None]: + """Copy skill directory to .github/skills/ and optionally .claude/skills/. + + This is a standalone function for direct skill copy operations. + It handles: + - Package type routing via should_install_skill() + - Skill name validation/normalization + - Directory structure preservation + - APM metadata injection for orphan detection + - Compatibility copy to .claude/skills/ when .claude/ exists (T7) + + Copies: + - SKILL.md (required) + - scripts/ (optional) + - references/ (optional) + - assets/ (optional) + - Any other subdirectories the package contains + + Args: + package_info: PackageInfo object with package metadata + source_path: Path to skill in apm_modules/ + target_base: Usually project root + + Returns: + Tuple of (github_path, claude_path): + - github_path: Path to .github/skills/{name}/ or None if skipped + - claude_path: Path to .claude/skills/{name}/ or None if .claude/ doesn't exist + """ + # Check if package type allows skill installation (T4 routing) + if not should_install_skill(package_info): + return (None, None) + + # Check for SKILL.md existence + source_skill_md = source_path / "SKILL.md" + if not source_skill_md.exists(): + # No SKILL.md means this package is handled by compilation, not skill copy + return (None, None) + + # Get and validate skill name from folder + raw_skill_name = source_path.name + + is_valid, error_msg = validate_skill_name(raw_skill_name) + if is_valid: + skill_name = raw_skill_name + else: + skill_name = normalize_skill_name(raw_skill_name) + + # === Primary target: .github/skills/ === + github_skill_dir = target_base / ".github" / "skills" / skill_name + + # Create .github/skills/ if it doesn't exist + github_skill_dir.parent.mkdir(parents=True, exist_ok=True) + + # If skill already exists, remove it for update + if github_skill_dir.exists(): + shutil.rmtree(github_skill_dir) + + # Copy the entire skill folder preserving structure + # This copies SKILL.md, scripts/, references/, assets/, etc. + shutil.copytree(source_path, github_skill_dir) + + # Add APM tracking metadata to SKILL.md for orphan detection + github_skill_md = github_skill_dir / "SKILL.md" + _add_apm_metadata(github_skill_md, package_info) + + # === Secondary target: .claude/skills/ (T7 - compatibility copy) === + claude_skill_dir: Path | None = None + claude_dir = target_base / ".claude" + + # Only copy to .claude/skills/ if .claude/ directory already exists + # Do NOT create .claude/ folder if it doesn't exist + if claude_dir.exists() and claude_dir.is_dir(): + claude_skill_dir = claude_dir / "skills" / skill_name + + # Create .claude/skills/ if needed (but .claude/ must already exist) + claude_skill_dir.parent.mkdir(parents=True, exist_ok=True) + + # If skill already exists, remove it for update + if claude_skill_dir.exists(): + shutil.rmtree(claude_skill_dir) + + # Copy the entire skill folder (identical to github copy) + shutil.copytree(source_path, claude_skill_dir) + + # Add APM tracking metadata + claude_skill_md = claude_skill_dir / "SKILL.md" + _add_apm_metadata(claude_skill_md, package_info) + + return (github_skill_dir, claude_skill_dir) + + +def _add_apm_metadata(skill_path: Path, package_info) -> None: + """Add APM tracking metadata to a SKILL.md file. + + This ensures sync_integration can identify APM-managed skills for cleanup. + + Args: + skill_path: Path to SKILL.md file + package_info: PackageInfo with package metadata + """ + from datetime import datetime + post = frontmatter.load(skill_path) + + # Add nested metadata for APM tracking + if 'metadata' not in post.metadata: + post.metadata['metadata'] = {} + + post.metadata['metadata']['apm_package'] = package_info.get_canonical_dependency_string() + post.metadata['metadata']['apm_version'] = package_info.package.version + post.metadata['metadata']['apm_commit'] = ( + package_info.resolved_reference.resolved_commit + if package_info.resolved_reference + else "unknown" + ) + post.metadata['metadata']['apm_installed_at'] = ( + package_info.installed_at or datetime.now().isoformat() + ) + + with open(skill_path, 'w', encoding='utf-8') as f: + f.write(frontmatter.dumps(post)) + + class SkillIntegrator: """Handles generation of SKILL.md files for Claude Code integration. @@ -464,20 +777,32 @@ def _generate_skill_file(self, package_info, primitives: dict, skill_dir: Path) return files_copied - def _integrate_native_claude_skill( + def _integrate_native_skill( self, package_info, project_root: Path, source_skill_md: Path ) -> SkillIntegrationResult: - """Copy a native Claude Skill (with existing SKILL.md) to .claude/skills/. + """Copy a native Skill (with existing SKILL.md) to .github/skills/ and optionally .claude/skills/. For packages that already have a SKILL.md at their root (like those from awesome-claude-skills), we copy the entire skill folder rather than regenerating from .apm/ primitives. - The skill folder name is the source folder name (e.g., `mcp-builder`). + The skill folder name is the source folder name (e.g., `mcp-builder`), + validated and normalized per the agentskills.io spec. We add APM tracking metadata to the copied SKILL.md so sync_integration can properly identify and clean up orphaned skills. + T7 Enhancement: Also copies to .claude/skills/ when .claude/ folder exists. + This ensures Claude Code users get skills while not polluting projects + that don't use Claude. + + Copies: + - SKILL.md (required) + - scripts/ (optional) + - references/ (optional) + - assets/ (optional) + - Any other subdirectories the package contains + Args: package_info: PackageInfo object with package metadata project_root: Root directory of the project @@ -490,19 +815,36 @@ def _integrate_native_claude_skill( # Use the source folder name as the skill name # e.g., apm_modules/ComposioHQ/awesome-claude-skills/mcp-builder → mcp-builder - skill_name = package_path.name + raw_skill_name = package_path.name + + # Validate skill name per agentskills.io spec + is_valid, error_msg = validate_skill_name(raw_skill_name) + if is_valid: + skill_name = raw_skill_name + else: + # Normalize the name if validation fails + skill_name = normalize_skill_name(raw_skill_name) + # Log warning about name normalization (import here to avoid circular import) + try: + from apm_cli.cli import _rich_warning + _rich_warning( + f"Skill name '{raw_skill_name}' normalized to '{skill_name}' ({error_msg})" + ) + except ImportError: + pass # CLI not available in tests - skill_dir = project_root / ".claude" / "skills" / skill_name - target_skill_md = skill_dir / "SKILL.md" + # Primary target: .github/skills/ + github_skill_dir = project_root / ".github" / "skills" / skill_name + github_skill_md = github_skill_dir / "SKILL.md" # Check if we need to update skill_created = False skill_updated = False skill_skipped = False - if target_skill_md.exists(): + if github_skill_md.exists(): # Check existing metadata for version/commit changes - existing_metadata = self._parse_skill_metadata(target_skill_md) + existing_metadata = self._parse_skill_metadata(github_skill_md) current_version = package_info.package.version current_commit = ( package_info.resolved_reference.resolved_commit @@ -519,26 +861,50 @@ def _integrate_native_claude_skill( skill_created = True files_copied = 0 + claude_skill_dir: Path | None = None if skill_created or skill_updated: + # === Copy to .github/skills/ (primary) === # Remove existing skill directory if updating - if skill_dir.exists(): - shutil.rmtree(skill_dir) + if github_skill_dir.exists(): + shutil.rmtree(github_skill_dir) + + # Create parent directory + github_skill_dir.parent.mkdir(parents=True, exist_ok=True) # Copy the entire package directory to the skill location - shutil.copytree(package_path, skill_dir) + shutil.copytree(package_path, github_skill_dir) # Add APM tracking metadata to the SKILL.md for orphan detection - self._add_apm_metadata_to_skill(target_skill_md, package_info) + self._add_apm_metadata_to_skill(github_skill_md, package_info) # Count files copied - files_copied = sum(1 for _ in skill_dir.rglob('*') if _.is_file()) + files_copied = sum(1 for _ in github_skill_dir.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(): + claude_skill_dir = claude_dir / "skills" / skill_name + + # Remove existing if updating + if claude_skill_dir.exists(): + shutil.rmtree(claude_skill_dir) + + # Create parent directory + claude_skill_dir.parent.mkdir(parents=True, exist_ok=True) + + # Copy the entire package directory (identical to github copy) + shutil.copytree(package_path, claude_skill_dir) + + # Add APM tracking metadata + claude_skill_md = claude_skill_dir / "SKILL.md" + self._add_apm_metadata_to_skill(claude_skill_md, package_info) return SkillIntegrationResult( skill_created=skill_created, skill_updated=skill_updated, skill_skipped=skill_skipped, - skill_path=target_skill_md if (skill_created or skill_updated) else None, + skill_path=github_skill_md if (skill_created or skill_updated) else None, references_copied=files_copied, links_resolved=0 ) @@ -569,13 +935,17 @@ def _add_apm_metadata_to_skill(self, skill_path: Path, package_info) -> None: f.write(frontmatter.dumps(post)) def integrate_package_skill(self, package_info, project_root: Path) -> SkillIntegrationResult: - """Generate SKILL.md for a package in .claude/skills/ directory. + """Generate SKILL.md for a package in .github/skills/ directory. Creates: - - .claude/skills/{skill-name}/SKILL.md - - .claude/skills/{skill-name}/references/ with prompt files + - .github/skills/{skill-name}/SKILL.md + - .github/skills/{skill-name}/references/ with prompt files + + This follows the Agent Skills standard (.github/skills/). - This follows Claude Code's skill discovery path (.claude/skills/). + Routing based on package type (from apm.yml): + - SKILL/HYBRID: Install as native skill + - INSTRUCTIONS/PROMPTS: Skip skill installation Note: Virtual FILE packages (individual files like owner/repo/path/to/file.agent.md) and COLLECTION packages do NOT generate Skills. Only full APM packages and @@ -591,6 +961,19 @@ def integrate_package_skill(self, package_info, project_root: Path) -> SkillInte Returns: SkillIntegrationResult: Results of the integration operation """ + # Check if package type allows skill installation (T4 routing) + # SKILL and HYBRID → install as skill + # INSTRUCTIONS and PROMPTS → skip skill installation + if not should_install_skill(package_info): + return SkillIntegrationResult( + skill_created=False, + skill_updated=False, + skill_skipped=True, + skill_path=None, + references_copied=0, + links_resolved=0 + ) + # Skip virtual FILE and COLLECTION packages - they're individual files, not full packages # Multiple virtual files from the same repo would collide on skill name # BUT: subdirectory packages (like Claude Skills) SHOULD generate skills @@ -608,10 +991,10 @@ def integrate_package_skill(self, package_info, project_root: Path) -> SkillInte package_path = package_info.install_path - # Check if this is a native Claude Skill (already has SKILL.md at root) + # Check if this is a native Skill (already has SKILL.md at root) source_skill_md = package_path / "SKILL.md" if source_skill_md.exists(): - return self._integrate_native_claude_skill(package_info, project_root, source_skill_md) + return self._integrate_native_skill(package_info, project_root, source_skill_md) # Discover all primitives for APM packages without SKILL.md instruction_files = self.find_instruction_files(package_path) @@ -640,11 +1023,11 @@ def integrate_package_skill(self, package_info, project_root: Path) -> SkillInte links_resolved=0 ) - # Determine target paths - write to .claude/skills/{skill-name}/ + # Determine target paths - write to .github/skills/{skill-name}/ # Use the install folder name for simplicity and consistency # e.g., apm_modules/danielmeppiel/design-guidelines → design-guidelines skill_name = package_path.name - skill_dir = project_root / ".claude" / "skills" / skill_name + skill_dir = project_root / ".github" / "skills" / skill_name skill_dir.mkdir(parents=True, exist_ok=True) skill_path = skill_dir / "SKILL.md" @@ -670,11 +1053,15 @@ def integrate_package_skill(self, package_info, project_root: Path) -> SkillInte ) files_copied = self._generate_skill_file(package_info, primitives, skill_dir) skill_updated = True + # T7: Also update .claude/skills/ if .claude/ exists + self._sync_to_claude_skills(package_info, primitives, skill_name, project_root) else: skill_skipped = True else: files_copied = self._generate_skill_file(package_info, primitives, skill_dir) skill_created = True + # T7: Also copy to .claude/skills/ if .claude/ exists + self._sync_to_claude_skills(package_info, primitives, skill_name, project_root) return SkillIntegrationResult( skill_created=skill_created, @@ -685,12 +1072,44 @@ def integrate_package_skill(self, package_info, project_root: Path) -> SkillInte links_resolved=0 # No longer tracking link resolution ) + def _sync_to_claude_skills( + self, package_info, primitives: dict, skill_name: str, project_root: Path + ) -> Path | None: + """Copy generated skill to .claude/skills/ if .claude/ directory exists. + + T7: Compatibility copy for Claude Code users. + Only copies if .claude/ folder already exists - does NOT create it. + + Args: + package_info: PackageInfo object with package metadata + primitives: Dict of primitive type -> list of files + skill_name: Normalized skill name + project_root: Root directory of the project + + Returns: + Path to .claude/skills/{name}/ or None if .claude/ doesn't exist + """ + claude_dir = project_root / ".claude" + if not claude_dir.exists() or not claude_dir.is_dir(): + return None + + # Create .claude/skills/{skill_name}/ + claude_skill_dir = claude_dir / "skills" / skill_name + claude_skill_dir.mkdir(parents=True, exist_ok=True) + + # Generate the skill file (identical to .github/skills/) + self._generate_skill_file(package_info, primitives, claude_skill_dir) + + return claude_skill_dir + def sync_integration(self, apm_package, project_root: Path) -> Dict[str, int]: - """Sync .claude/skills/ with currently installed packages. + """Sync .github/skills/ and .claude/skills/ with currently installed packages. Removes skill directories for packages that are no longer installed. Uses apm_package metadata in SKILL.md to identify APM-managed skills. + T7 Enhancement: Cleans both .github/skills/ and .claude/skills/ locations. + Args: apm_package: APMPackage with current dependencies project_root: Root directory of the project @@ -698,16 +1117,39 @@ def sync_integration(self, apm_package, project_root: Path) -> Dict[str, int]: Returns: Dict with cleanup statistics """ - skills_dir = project_root / ".claude" / "skills" - if not skills_dir.exists(): - return {'files_removed': 0, 'errors': 0} + stats = {'files_removed': 0, 'errors': 0} # Get canonical dependency strings for all installed packages installed_packages = set() for dep in apm_package.get_apm_dependencies(): installed_packages.add(dep.get_canonical_dependency_string()) - # Find orphaned skill directories by checking apm_package metadata + # Clean .github/skills/ (primary) + github_skills_dir = project_root / ".github" / "skills" + if github_skills_dir.exists(): + result = self._clean_orphaned_skills(github_skills_dir, installed_packages) + stats['files_removed'] += result['files_removed'] + stats['errors'] += result['errors'] + + # Clean .claude/skills/ (secondary - T7 compatibility) + claude_skills_dir = project_root / ".claude" / "skills" + if claude_skills_dir.exists(): + result = self._clean_orphaned_skills(claude_skills_dir, installed_packages) + stats['files_removed'] += result['files_removed'] + stats['errors'] += result['errors'] + + return stats + + def _clean_orphaned_skills(self, skills_dir: Path, installed_packages: set) -> Dict[str, int]: + """Clean orphaned skills from a skills directory. + + Args: + skills_dir: Path to skills directory (.github/skills/ or .claude/skills/) + installed_packages: Set of canonical dependency strings for installed packages + + Returns: + Dict with cleanup statistics + """ files_removed = 0 errors = 0 @@ -740,8 +1182,8 @@ def update_gitignore_for_skills(self, project_root: Path) -> bool: gitignore_path = project_root / ".gitignore" patterns = [ - ".claude/skills/*-apm/", # APM-generated skills use -apm suffix - "# APM-generated Claude skills" + ".github/skills/*-apm/", # APM-generated skills use -apm suffix + "# APM-generated skills" ] # Read current content diff --git a/src/apm_cli/models/__init__.py b/src/apm_cli/models/__init__.py index f7b2d2084..662c4df8d 100644 --- a/src/apm_cli/models/__init__.py +++ b/src/apm_cli/models/__init__.py @@ -8,6 +8,7 @@ ResolvedReference, PackageInfo, GitReferenceType, + PackageContentType, ) __all__ = [ @@ -18,4 +19,5 @@ "ResolvedReference", "PackageInfo", "GitReferenceType", + "PackageContentType", ] \ No newline at end of file diff --git a/src/apm_cli/models/apm_package.py b/src/apm_cli/models/apm_package.py index b77549225..cb955ad8d 100644 --- a/src/apm_cli/models/apm_package.py +++ b/src/apm_cli/models/apm_package.py @@ -18,13 +18,60 @@ class GitReferenceType(Enum): class PackageType(Enum): - """Types of packages that APM can install.""" + """Types of packages that APM can install. + + This enum is used internally to classify packages based on their content + (presence of apm.yml, SKILL.md, etc.). + """ APM_PACKAGE = "apm_package" # Has apm.yml CLAUDE_SKILL = "claude_skill" # Has SKILL.md, no apm.yml HYBRID = "hybrid" # Has both apm.yml and SKILL.md INVALID = "invalid" # Neither apm.yml nor SKILL.md +class PackageContentType(Enum): + """Explicit package content type declared in apm.yml. + + This is the user-facing `type` field in apm.yml that controls how the + package is processed during install/compile: + - INSTRUCTIONS: Compile to AGENTS.md only, no skill created + - SKILL: Install as native skill only, no AGENTS.md compilation + - HYBRID: Both AGENTS.md instructions AND skill installation (default) + - PROMPTS: Commands/prompts only, no instructions or skills + """ + INSTRUCTIONS = "instructions" # Compile to AGENTS.md only + SKILL = "skill" # Install as native skill only + HYBRID = "hybrid" # Both (default) + PROMPTS = "prompts" # Commands/prompts only + + @classmethod + def from_string(cls, value: str) -> "PackageContentType": + """Parse a string value into a PackageContentType enum. + + Args: + value: String value to parse (e.g., "instructions", "skill") + + Returns: + PackageContentType: The corresponding enum value + + Raises: + ValueError: If the value is not a valid package content type + """ + if not value: + raise ValueError("Package type cannot be empty") + + value_lower = value.lower().strip() + for member in cls: + if member.value == value_lower: + return member + + valid_types = ", ".join(f"'{m.value}'" for m in cls) + raise ValueError( + f"Invalid package type '{value}'. " + f"Valid types are: {valid_types}" + ) + + class ValidationError(Enum): """Types of validation errors for APM packages.""" MISSING_APM_YML = "missing_apm_yml" @@ -647,6 +694,7 @@ class APMPackage: scripts: Optional[Dict[str, str]] = None package_path: Optional[Path] = None # Local path to package target: Optional[str] = None # Target agent: vscode, claude, or all (applies to compile and install) + type: Optional[PackageContentType] = None # Package content type: instructions, skill, hybrid, or prompts @classmethod def from_apm_yml(cls, apm_yml_path: Path) -> "APMPackage": @@ -700,6 +748,17 @@ def from_apm_yml(cls, apm_yml_path: Path) -> "APMPackage": # Other dependencies (like MCP) remain as strings dependencies[dep_type] = [str(dep) for dep in dep_list if isinstance(dep, str)] + # Parse package content type + pkg_type = None + if 'type' in data and data['type'] is not None: + type_value = data['type'] + if not isinstance(type_value, str): + raise ValueError(f"Invalid 'type' field: expected string, got {type(type_value).__name__}") + try: + pkg_type = PackageContentType.from_string(type_value) + except ValueError as e: + raise ValueError(f"Invalid 'type' field in apm.yml: {e}") + return cls( name=data['name'], version=data['version'], @@ -710,6 +769,7 @@ def from_apm_yml(cls, apm_yml_path: Path) -> "APMPackage": scripts=data.get('scripts'), package_path=apm_yml_path.parent, target=data.get('target'), + type=pkg_type, ) def get_apm_dependencies(self) -> List[DependencyReference]: diff --git a/tests/test_apm_package_models.py b/tests/test_apm_package_models.py index 88f93a405..b7910a3e5 100644 --- a/tests/test_apm_package_models.py +++ b/tests/test_apm_package_models.py @@ -14,6 +14,7 @@ ResolvedReference, PackageInfo, GitReferenceType, + PackageContentType, validate_apm_package, parse_git_reference, ) @@ -827,4 +828,238 @@ def test_has_primitives(self): # .apm with primitive files (apm_dir / "instructions" / "test.md").write_text("# Test") - assert info.has_primitives() \ No newline at end of file + assert info.has_primitives() + + +class TestPackageContentType: + """Test PackageContentType enum and parsing.""" + + def test_enum_values(self): + """Test that all expected enum values exist.""" + assert PackageContentType.INSTRUCTIONS.value == "instructions" + assert PackageContentType.SKILL.value == "skill" + assert PackageContentType.HYBRID.value == "hybrid" + assert PackageContentType.PROMPTS.value == "prompts" + + def test_from_string_valid_values(self): + """Test parsing all valid type values.""" + assert PackageContentType.from_string("instructions") == PackageContentType.INSTRUCTIONS + assert PackageContentType.from_string("skill") == PackageContentType.SKILL + assert PackageContentType.from_string("hybrid") == PackageContentType.HYBRID + assert PackageContentType.from_string("prompts") == PackageContentType.PROMPTS + + def test_from_string_case_insensitive(self): + """Test that parsing is case-insensitive.""" + assert PackageContentType.from_string("INSTRUCTIONS") == PackageContentType.INSTRUCTIONS + assert PackageContentType.from_string("Skill") == PackageContentType.SKILL + assert PackageContentType.from_string("HYBRID") == PackageContentType.HYBRID + assert PackageContentType.from_string("Prompts") == PackageContentType.PROMPTS + + def test_from_string_with_whitespace(self): + """Test that parsing handles leading/trailing whitespace.""" + assert PackageContentType.from_string(" instructions ") == PackageContentType.INSTRUCTIONS + assert PackageContentType.from_string("\tskill\n") == PackageContentType.SKILL + + def test_from_string_invalid_value(self): + """Test that invalid values raise ValueError with helpful message.""" + with pytest.raises(ValueError) as exc_info: + PackageContentType.from_string("invalid") + + error_msg = str(exc_info.value) + assert "Invalid package type 'invalid'" in error_msg + assert "'instructions'" in error_msg + assert "'skill'" in error_msg + assert "'hybrid'" in error_msg + assert "'prompts'" in error_msg + + def test_from_string_empty_value(self): + """Test that empty string raises ValueError.""" + with pytest.raises(ValueError, match="Package type cannot be empty"): + PackageContentType.from_string("") + + def test_from_string_typo_suggestions(self): + """Test helpful error message for common typos.""" + # Test that error message lists all valid types + with pytest.raises(ValueError) as exc_info: + PackageContentType.from_string("instruction") # Missing 's' + + error_msg = str(exc_info.value) + assert "'instructions'" in error_msg # Shows correct spelling + + +class TestAPMPackageTypeField: + """Test APMPackage type field parsing from apm.yml.""" + + def test_type_field_instructions(self): + """Test parsing type: instructions from apm.yml.""" + apm_content = { + 'name': 'test-package', + 'version': '1.0.0', + 'type': 'instructions' + } + + with tempfile.NamedTemporaryFile(mode='w', suffix='.yml', delete=False) as f: + yaml.dump(apm_content, f) + f.flush() + + package = APMPackage.from_apm_yml(Path(f.name)) + assert package.type == PackageContentType.INSTRUCTIONS + + Path(f.name).unlink() + + def test_type_field_skill(self): + """Test parsing type: skill from apm.yml.""" + apm_content = { + 'name': 'test-package', + 'version': '1.0.0', + 'type': 'skill' + } + + with tempfile.NamedTemporaryFile(mode='w', suffix='.yml', delete=False) as f: + yaml.dump(apm_content, f) + f.flush() + + package = APMPackage.from_apm_yml(Path(f.name)) + assert package.type == PackageContentType.SKILL + + Path(f.name).unlink() + + def test_type_field_hybrid(self): + """Test parsing type: hybrid from apm.yml.""" + apm_content = { + 'name': 'test-package', + 'version': '1.0.0', + 'type': 'hybrid' + } + + with tempfile.NamedTemporaryFile(mode='w', suffix='.yml', delete=False) as f: + yaml.dump(apm_content, f) + f.flush() + + package = APMPackage.from_apm_yml(Path(f.name)) + assert package.type == PackageContentType.HYBRID + + Path(f.name).unlink() + + def test_type_field_prompts(self): + """Test parsing type: prompts from apm.yml.""" + apm_content = { + 'name': 'test-package', + 'version': '1.0.0', + 'type': 'prompts' + } + + with tempfile.NamedTemporaryFile(mode='w', suffix='.yml', delete=False) as f: + yaml.dump(apm_content, f) + f.flush() + + package = APMPackage.from_apm_yml(Path(f.name)) + assert package.type == PackageContentType.PROMPTS + + Path(f.name).unlink() + + def test_type_field_missing_defaults_to_none(self): + """Test that missing type field defaults to None (hybrid behavior).""" + apm_content = { + 'name': 'test-package', + 'version': '1.0.0' + } + + with tempfile.NamedTemporaryFile(mode='w', suffix='.yml', delete=False) as f: + yaml.dump(apm_content, f) + f.flush() + + package = APMPackage.from_apm_yml(Path(f.name)) + assert package.type is None # Default to None for backward compatibility + + Path(f.name).unlink() + + def test_type_field_invalid_raises_error(self): + """Test that invalid type value raises ValueError.""" + apm_content = { + 'name': 'test-package', + 'version': '1.0.0', + 'type': 'invalid-type' + } + + with tempfile.NamedTemporaryFile(mode='w', suffix='.yml', delete=False) as f: + yaml.dump(apm_content, f) + f.flush() + + with pytest.raises(ValueError) as exc_info: + APMPackage.from_apm_yml(Path(f.name)) + + error_msg = str(exc_info.value) + assert "Invalid 'type' field" in error_msg + assert "invalid-type" in error_msg + + Path(f.name).unlink() + + def test_type_field_non_string_raises_error(self): + """Test that non-string type value raises ValueError.""" + apm_content = { + 'name': 'test-package', + 'version': '1.0.0', + 'type': 123 # Numeric type + } + + with tempfile.NamedTemporaryFile(mode='w', suffix='.yml', delete=False) as f: + yaml.dump(apm_content, f) + f.flush() + + with pytest.raises(ValueError) as exc_info: + APMPackage.from_apm_yml(Path(f.name)) + + error_msg = str(exc_info.value) + assert "expected string" in error_msg + assert "int" in error_msg + + Path(f.name).unlink() + + def test_type_field_case_insensitive_in_yaml(self): + """Test that type field parsing is case-insensitive in YAML.""" + apm_content = { + 'name': 'test-package', + 'version': '1.0.0', + 'type': 'SKILL' # Uppercase + } + + with tempfile.NamedTemporaryFile(mode='w', suffix='.yml', delete=False) as f: + yaml.dump(apm_content, f) + f.flush() + + package = APMPackage.from_apm_yml(Path(f.name)) + assert package.type == PackageContentType.SKILL + + Path(f.name).unlink() + + def test_type_field_null_treated_as_missing(self): + """Test that explicit null type field is treated as missing.""" + # Write YAML directly to handle null explicitly + yaml_content = """name: test-package +version: "1.0.0" +type: null +""" + + with tempfile.NamedTemporaryFile(mode='w', suffix='.yml', delete=False) as f: + f.write(yaml_content) + f.flush() + + package = APMPackage.from_apm_yml(Path(f.name)) + assert package.type is None + + Path(f.name).unlink() + + def test_package_dataclass_with_type(self): + """Test that APMPackage dataclass accepts type parameter.""" + package = APMPackage( + name="test", + version="1.0.0", + type=PackageContentType.SKILL + ) + assert package.type == PackageContentType.SKILL + + def test_package_dataclass_type_defaults_to_none(self): + """Test that APMPackage type defaults to None when not provided.""" + package = APMPackage(name="test", version="1.0.0") + assert package.type is None \ No newline at end of file diff --git a/tests/unit/integration/test_agent_integrator.py b/tests/unit/integration/test_agent_integrator.py index ca0f9d9d5..6b487db50 100644 --- a/tests/unit/integration/test_agent_integrator.py +++ b/tests/unit/integration/test_agent_integrator.py @@ -751,6 +751,87 @@ def test_sync_integration_handles_files_without_metadata(self): # File without header should be preserved (not removed) - it's a user's file assert (github_agents / "custom-apm.agent.md").exists() + # ========== Skill Separation Regression Tests (T5) ========== + # ARCHITECTURE DECISION: Skills are NOT Agents + # Skills go to .github/skills/ via SkillIntegrator + # Agents go to .github/agents/ via AgentIntegrator + # These tests verify agent_integrator does NOT transform skills + + def test_skill_files_not_converted_to_agents(self): + """Regression test: SKILL.md files must NOT be transformed to .agent.md. + + This was removed in T5 of the Skills Strategy refactoring. + Skills and Agents have different semantics: + - Skills: Declarative context/knowledge packages (.github/skills/) + - Agents: Executable VSCode chat modes (.github/agents/) + """ + package_dir = self.project_root / "package" + package_dir.mkdir() + + # Create a SKILL.md file + (package_dir / "SKILL.md").write_text("""--- +name: test-skill +description: A test skill +--- +# Test Skill + +This is a skill, not an agent.""") + + github_dir = self.project_root / ".github" + github_dir.mkdir() + + package = APMPackage( + name="skill-pkg", + version="1.0.0", + package_path=package_dir + ) + resolved_ref = ResolvedReference( + original_ref="main", + ref_type=GitReferenceType.BRANCH, + resolved_commit="abc123", + ref_name="main" + ) + package_info = PackageInfo( + package=package, + install_path=package_dir, + resolved_reference=resolved_ref, + installed_at=datetime.now().isoformat() + ) + + result = self.integrator.integrate_package_agents(package_info, self.project_root) + + # No agents should be created from skills + assert result.files_integrated == 0 + + # Verify .github/agents/ does NOT contain skill-derived files + agents_dir = self.project_root / ".github" / "agents" + if agents_dir.exists(): + agent_files = list(agents_dir.glob("*.agent.md")) + for agent_file in agent_files: + assert "skill" not in agent_file.name.lower(), \ + f"SKILL.md was incorrectly transformed to agent: {agent_file}" + + def test_find_agent_files_ignores_skill_files(self): + """AgentIntegrator.find_agent_files() must not find SKILL.md files.""" + package_dir = self.project_root / "package" + package_dir.mkdir() + + # Create various files + (package_dir / "security.agent.md").write_text("# Real Agent") + (package_dir / "SKILL.md").write_text("# This is a skill") + (package_dir / "skill.md").write_text("# Also a skill") + + agents = self.integrator.find_agent_files(package_dir) + + # Only .agent.md files should be found + assert len(agents) == 1 + assert agents[0].name == "security.agent.md" + + # Verify no SKILL.md files were picked up + found_names = [a.name for a in agents] + assert "SKILL.md" not in found_names + assert "skill.md" not in found_names + class TestAgentSuffixPattern: """Test -apm suffix pattern edge cases for agents.""" diff --git a/tests/unit/integration/test_skill_integrator.py b/tests/unit/integration/test_skill_integrator.py index d0d69c142..3ddf241e3 100644 --- a/tests/unit/integration/test_skill_integrator.py +++ b/tests/unit/integration/test_skill_integrator.py @@ -6,8 +6,8 @@ from unittest.mock import Mock from datetime import datetime -from apm_cli.integration.skill_integrator import SkillIntegrator, SkillIntegrationResult, to_hyphen_case -from apm_cli.models.apm_package import PackageInfo, APMPackage, ResolvedReference, GitReferenceType, DependencyReference +from apm_cli.integration.skill_integrator import SkillIntegrator, SkillIntegrationResult, to_hyphen_case, validate_skill_name, normalize_skill_name, copy_skill_to_target +from apm_cli.models.apm_package import PackageInfo, APMPackage, ResolvedReference, GitReferenceType, DependencyReference, PackageType, PackageContentType class TestToHyphenCase: @@ -96,7 +96,7 @@ def _get_skill_path(self, package_info) -> Path: Uses the install folder name for simplicity and consistency. """ skill_name = package_info.install_path.name - return self.project_root / ".claude" / "skills" / skill_name + return self.project_root / ".github" / "skills" / skill_name # ========== should_integrate tests ========== @@ -360,15 +360,23 @@ def _create_package_info( install_path: Path = None, source: str = None, description: str = None, - dependency_ref: DependencyReference = None + dependency_ref: DependencyReference = None, + package_type: PackageType = None, + content_type: "PackageContentType" = None ) -> PackageInfo: - """Helper to create PackageInfo objects for tests.""" + """Helper to create PackageInfo objects for tests. + + Args: + package_type: Internal detection type (CLAUDE_SKILL, HYBRID, APM_PACKAGE) + content_type: Explicit type from apm.yml (skill, hybrid, instructions, prompts) + """ package = APMPackage( name=name, version=version, package_path=install_path or self.project_root / "package", source=source or f"github.com/test/{name}", - description=description + description=description, + type=content_type ) resolved_ref = ResolvedReference( original_ref="main", @@ -381,17 +389,26 @@ def _create_package_info( install_path=install_path or self.project_root / "package", resolved_reference=resolved_ref, installed_at=datetime.now().isoformat(), - dependency_ref=dependency_ref + dependency_ref=dependency_ref, + package_type=package_type ) def test_integrate_package_skill_creates_skill_md(self): - """Test that SKILL.md is created when package has content.""" + """Test that SKILL.md is created when package has content and type=HYBRID. + + Per skill-strategy.md Decision 2: Skills are explicit, not implicit. + Packages with primitives only become skills if they declare type: hybrid. + """ package_dir = self.project_root / "package" apm_instructions = package_dir / ".apm" / "instructions" apm_instructions.mkdir(parents=True) (apm_instructions / "coding.instructions.md").write_text("# Coding Guidelines") - package_info = self._create_package_info(install_path=package_dir) + # Must set HYBRID type - primitives alone don't auto-become skills + package_info = self._create_package_info( + install_path=package_dir, + package_type=PackageType.HYBRID + ) skill_dir = self._get_skill_path(package_info) result = self.integrator.integrate_package_skill(package_info, self.project_root) @@ -452,7 +469,7 @@ def test_integrate_package_skill_skips_virtual_file_packages(self): assert result.skill_skipped is True assert result.skill_path is None # No skill directory should be created - skill_dir = self.project_root / ".claude" / "skills" / "awesome-copilot" + skill_dir = self.project_root / ".github" / "skills" / "awesome-copilot" assert not skill_dir.exists() def test_integrate_package_skill_processes_virtual_subdirectory_packages(self): @@ -475,11 +492,13 @@ def test_integrate_package_skill_processes_virtual_subdirectory_packages(self): assert virtual_dep_ref.is_virtual # Sanity check assert virtual_dep_ref.is_virtual_subdirectory() # This is a subdirectory, not file + # Has SKILL.md → CLAUDE_SKILL type package_info = self._create_package_info( install_path=package_dir, name="mcp-builder", source="ComposioHQ/awesome-claude-skills", - dependency_ref=virtual_dep_ref + dependency_ref=virtual_dep_ref, + package_type=PackageType.CLAUDE_SKILL ) result = self.integrator.integrate_package_skill(package_info, self.project_root) @@ -530,16 +549,22 @@ def test_integrate_package_skill_multiple_virtual_file_packages_no_collision(sel assert result2.skill_skipped is True # No skill directories should exist - skills_dir = self.project_root / ".claude" / "skills" + skills_dir = self.project_root / ".github" / "skills" assert not skills_dir.exists() def test_integrate_package_skill_creates_prompts_subdirectory(self): - """Test that prompts subdirectory is created with prompt files.""" + """Test that prompts subdirectory is created with prompt files. + + Per skill-strategy.md: prompts-only packages need explicit type: hybrid to become skills. + """ package_dir = self.project_root / "package" package_dir.mkdir() (package_dir / "test.prompt.md").write_text("# Test Prompt") - package_info = self._create_package_info(install_path=package_dir) + package_info = self._create_package_info( + install_path=package_dir, + package_type=PackageType.HYBRID + ) skill_dir = self._get_skill_path(package_info) result = self.integrator.integrate_package_skill(package_info, self.project_root) @@ -561,7 +586,8 @@ def test_integrate_package_skill_yaml_frontmatter_has_required_fields(self): version="2.0.0", commit="def456", install_path=package_dir, - description="A test package" + description="A test package", + package_type=PackageType.HYBRID ) skill_dir = self._get_skill_path(package_info) @@ -590,7 +616,8 @@ def test_integrate_package_skill_name_follows_hyphen_case(self): package_info = self._create_package_info( name="MyAwesomePackage", install_path=package_dir, - source="github.com/owner/MyAwesomePackage" + source="github.com/owner/MyAwesomePackage", + package_type=PackageType.HYBRID ) skill_dir = self._get_skill_path(package_info) @@ -608,7 +635,10 @@ def test_integrate_package_skill_includes_instructions_section(self): apm_instructions.mkdir(parents=True) (apm_instructions / "coding.instructions.md").write_text("Follow coding standards") - package_info = self._create_package_info(install_path=package_dir) + package_info = self._create_package_info( + install_path=package_dir, + package_type=PackageType.HYBRID + ) skill_dir = self._get_skill_path(package_info) self.integrator.integrate_package_skill(package_info, self.project_root) @@ -630,7 +660,10 @@ def test_integrate_package_skill_includes_agents_section(self): apm_agents.mkdir(parents=True) (apm_agents / "reviewer.agent.md").write_text("Review code for quality") - package_info = self._create_package_info(install_path=package_dir) + package_info = self._create_package_info( + install_path=package_dir, + package_type=PackageType.HYBRID + ) skill_dir = self._get_skill_path(package_info) self.integrator.integrate_package_skill(package_info, self.project_root) @@ -650,7 +683,10 @@ def test_integrate_package_skill_includes_prompts_section(self): package_dir.mkdir() (package_dir / "design-review.prompt.md").write_text("# Design Review") - package_info = self._create_package_info(install_path=package_dir) + package_info = self._create_package_info( + install_path=package_dir, + package_type=PackageType.HYBRID + ) skill_dir = self._get_skill_path(package_info) self.integrator.integrate_package_skill(package_info, self.project_root) @@ -673,7 +709,8 @@ def test_integrate_package_skill_updates_when_version_changes(self): package_info = self._create_package_info( version="2.0.0", commit="abc123", - install_path=package_dir + install_path=package_dir, + package_type=PackageType.HYBRID ) skill_dir = self._get_skill_path(package_info) skill_dir.mkdir(parents=True, exist_ok=True) @@ -714,7 +751,8 @@ def test_integrate_package_skill_updates_when_commit_changes(self): package_info = self._create_package_info( version="1.0.0", commit="def456", # New commit - install_path=package_dir + install_path=package_dir, + package_type=PackageType.HYBRID ) skill_dir = self._get_skill_path(package_info) skill_dir.mkdir(parents=True, exist_ok=True) @@ -780,12 +818,18 @@ def test_integrate_package_skill_skips_when_unchanged(self): assert result.skill_skipped is True def test_integrate_package_skill_with_only_prompts(self): - """Test integration works with only prompt files.""" + """Test integration works with only prompt files. + + Per skill-strategy.md: prompts-only packages need explicit type: hybrid. + """ package_dir = self.project_root / "package" package_dir.mkdir() (package_dir / "review.prompt.md").write_text("# Review Prompt") - package_info = self._create_package_info(install_path=package_dir) + package_info = self._create_package_info( + install_path=package_dir, + package_type=PackageType.HYBRID + ) skill_dir = self._get_skill_path(package_info) result = self.integrator.integrate_package_skill(package_info, self.project_root) @@ -795,13 +839,19 @@ def test_integrate_package_skill_with_only_prompts(self): assert (skill_dir / "SKILL.md").exists() def test_integrate_package_skill_with_only_context(self): - """Test integration works with only context files.""" + """Test integration works with only context files. + + Per skill-strategy.md: context-only packages need explicit type: hybrid. + """ package_dir = self.project_root / "package" apm_context = package_dir / ".apm" / "context" apm_context.mkdir(parents=True) (apm_context / "project.context.md").write_text("# Project Context") - package_info = self._create_package_info(install_path=package_dir) + package_info = self._create_package_info( + install_path=package_dir, + package_type=PackageType.HYBRID + ) skill_dir = self._get_skill_path(package_info) result = self.integrator.integrate_package_skill(package_info, self.project_root) @@ -824,7 +874,8 @@ def test_skill_md_description_truncated_to_1024_chars(self): long_description = "A" * 2000 # Longer than 1024 limit package_info = self._create_package_info( install_path=package_dir, - description=long_description + description=long_description, + package_type=PackageType.HYBRID ) skill_dir = self._get_skill_path(package_info) @@ -844,7 +895,10 @@ def test_skill_md_includes_content_hash(self): apm_instructions.mkdir(parents=True) (apm_instructions / "test.instructions.md").write_text("# Test Content") - package_info = self._create_package_info(install_path=package_dir) + package_info = self._create_package_info( + install_path=package_dir, + package_type=PackageType.HYBRID + ) skill_dir = self._get_skill_path(package_info) self.integrator.integrate_package_skill(package_info, self.project_root) @@ -863,12 +917,12 @@ def test_update_gitignore_adds_skill_patterns(self): assert updated is True content = gitignore.read_text() - assert ".claude/skills/*-apm/" in content + assert ".github/skills/*-apm/" in content def test_update_gitignore_skips_if_patterns_exist(self): """Test that gitignore update is skipped if patterns already exist.""" gitignore = self.project_root / ".gitignore" - gitignore.write_text(".claude/skills/*-apm/\n# APM-generated Claude skills\n") + gitignore.write_text(".github/skills/*-apm/\n# APM-generated skills\n") updated = self.integrator.update_gitignore_for_skills(self.project_root) @@ -882,7 +936,7 @@ def test_update_gitignore_creates_file_if_missing(self): gitignore = self.project_root / ".gitignore" assert gitignore.exists() content = gitignore.read_text() - assert ".claude/skills/*-apm/" in content + assert ".github/skills/*-apm/" in content # ========== sync_integration tests ========== @@ -904,7 +958,7 @@ def test_sync_integration_removes_orphaned_subdirectory_skill(self): # Simulate an installed skill from a subdirectory package # Skill name uses the folder name directly: mcp-builder skill_name = "mcp-builder" - skill_dir = self.project_root / ".claude" / "skills" / skill_name + skill_dir = self.project_root / ".github" / "skills" / skill_name skill_dir.mkdir(parents=True) # Create SKILL.md with APM metadata (matching _generate_skill_file's nested format) @@ -934,7 +988,7 @@ def test_sync_integration_keeps_installed_subdirectory_skill(self): # Simulate an installed skill from a subdirectory package # Skill name uses the folder name directly: mcp-builder skill_name = "mcp-builder" - skill_dir = self.project_root / ".claude" / "skills" / skill_name + skill_dir = self.project_root / ".github" / "skills" / skill_name skill_dir.mkdir(parents=True) # Create SKILL.md with APM metadata (matching _generate_skill_file's nested format) @@ -979,7 +1033,10 @@ def test_integrate_handles_frontmatter_in_source_files(self): This is the content.""" (apm_instructions / "test.instructions.md").write_text(content_with_frontmatter) - package_info = self._create_package_info(install_path=package_dir) + package_info = self._create_package_info( + install_path=package_dir, + package_type=PackageType.HYBRID + ) skill_dir = self._get_skill_path(package_info) self.integrator.integrate_package_skill(package_info, self.project_root) @@ -1010,7 +1067,10 @@ def test_integrate_with_multiple_primitive_types(self): (apm_context / "project.context.md").write_text("# Project") (package_dir / "workflow.prompt.md").write_text("# Workflow") - package_info = self._create_package_info(install_path=package_dir) + package_info = self._create_package_info( + install_path=package_dir, + package_type=PackageType.HYBRID + ) skill_dir = self._get_skill_path(package_info) result = self.integrator.integrate_package_skill(package_info, self.project_root) @@ -1064,3 +1124,1583 @@ def test_result_with_values(self): assert result.skill_path == skill_path assert result.references_copied == 3 assert result.links_resolved == 5 + + +class TestValidateSkillName: + """Test skill name validation per agentskills.io spec.""" + + # ========== Valid names ========== + + def test_valid_simple_lowercase(self): + """Test valid simple lowercase name.""" + is_valid, error = validate_skill_name("mypackage") + assert is_valid is True + assert error == "" + + def test_valid_with_hyphens(self): + """Test valid name with hyphens.""" + is_valid, error = validate_skill_name("my-awesome-package") + assert is_valid is True + assert error == "" + + def test_valid_with_numbers(self): + """Test valid name with numbers.""" + is_valid, error = validate_skill_name("package123") + assert is_valid is True + assert error == "" + + def test_valid_numbers_and_hyphens(self): + """Test valid name with numbers and hyphens.""" + is_valid, error = validate_skill_name("my-package-2") + assert is_valid is True + assert error == "" + + def test_valid_single_char(self): + """Test valid single character name.""" + is_valid, error = validate_skill_name("a") + assert is_valid is True + assert error == "" + + def test_valid_single_number(self): + """Test valid single number name.""" + is_valid, error = validate_skill_name("1") + assert is_valid is True + assert error == "" + + def test_valid_64_chars(self): + """Test valid name at max length (64 chars).""" + name = "a" * 64 + is_valid, error = validate_skill_name(name) + assert is_valid is True + assert error == "" + + def test_valid_realistic_names(self): + """Test valid realistic skill names.""" + valid_names = [ + "mcp-builder", + "brand-guidelines", + "code-review", + "gdpr-assessment", + "python-standards", + "react-components", + "aws-lambda-v2", + "openai-gpt4o", + ] + for name in valid_names: + is_valid, error = validate_skill_name(name) + assert is_valid is True, f"Expected '{name}' to be valid, got error: {error}" + + # ========== Invalid: Uppercase letters ========== + + def test_invalid_uppercase(self): + """Test invalid name with uppercase letters.""" + is_valid, error = validate_skill_name("MyPackage") + assert is_valid is False + assert "lowercase" in error.lower() + + def test_invalid_all_uppercase(self): + """Test invalid name with all uppercase.""" + is_valid, error = validate_skill_name("MYPACKAGE") + assert is_valid is False + assert "lowercase" in error.lower() + + def test_invalid_mixed_case(self): + """Test invalid name with mixed case.""" + is_valid, error = validate_skill_name("myPackage") + assert is_valid is False + assert "lowercase" in error.lower() + + # ========== Invalid: Underscores ========== + + def test_invalid_underscore(self): + """Test invalid name with underscores.""" + is_valid, error = validate_skill_name("my_package") + assert is_valid is False + assert "underscore" in error.lower() + + def test_invalid_multiple_underscores(self): + """Test invalid name with multiple underscores.""" + is_valid, error = validate_skill_name("my_awesome_package") + assert is_valid is False + assert "underscore" in error.lower() + + # ========== Invalid: Spaces ========== + + def test_invalid_space(self): + """Test invalid name with spaces.""" + is_valid, error = validate_skill_name("my package") + assert is_valid is False + assert "space" in error.lower() + + def test_invalid_multiple_spaces(self): + """Test invalid name with multiple spaces.""" + is_valid, error = validate_skill_name("my awesome package") + assert is_valid is False + assert "space" in error.lower() + + # ========== Invalid: Special characters ========== + + def test_invalid_special_chars(self): + """Test invalid name with special characters.""" + is_valid, error = validate_skill_name("my@package") + assert is_valid is False + assert "invalid character" in error.lower() or "alphanumeric" in error.lower() + + def test_invalid_dots(self): + """Test invalid name with dots.""" + is_valid, error = validate_skill_name("my.package") + assert is_valid is False + assert "invalid character" in error.lower() or "alphanumeric" in error.lower() + + def test_invalid_slashes(self): + """Test invalid name with slashes.""" + is_valid, error = validate_skill_name("my/package") + assert is_valid is False + assert "invalid character" in error.lower() or "alphanumeric" in error.lower() + + # ========== Invalid: Consecutive hyphens ========== + + def test_invalid_consecutive_hyphens(self): + """Test invalid name with consecutive hyphens.""" + is_valid, error = validate_skill_name("my--package") + assert is_valid is False + assert "consecutive" in error.lower() + + def test_invalid_triple_hyphens(self): + """Test invalid name with triple hyphens.""" + is_valid, error = validate_skill_name("my---package") + assert is_valid is False + assert "consecutive" in error.lower() + + def test_invalid_multiple_consecutive_groups(self): + """Test invalid name with multiple groups of consecutive hyphens.""" + is_valid, error = validate_skill_name("my--awesome--package") + assert is_valid is False + assert "consecutive" in error.lower() + + # ========== Invalid: Leading/trailing hyphens ========== + + def test_invalid_leading_hyphen(self): + """Test invalid name starting with hyphen.""" + is_valid, error = validate_skill_name("-mypackage") + assert is_valid is False + assert "start" in error.lower() + + def test_invalid_trailing_hyphen(self): + """Test invalid name ending with hyphen.""" + is_valid, error = validate_skill_name("mypackage-") + assert is_valid is False + assert "end" in error.lower() + + def test_invalid_both_leading_trailing_hyphens(self): + """Test invalid name with both leading and trailing hyphens.""" + is_valid, error = validate_skill_name("-mypackage-") + assert is_valid is False + # Either error is acceptable + assert "start" in error.lower() or "end" in error.lower() + + def test_invalid_only_hyphen(self): + """Test invalid name that is just a hyphen.""" + is_valid, error = validate_skill_name("-") + assert is_valid is False + assert "start" in error.lower() + + # ========== Invalid: Length ========== + + def test_invalid_empty_string(self): + """Test invalid empty name.""" + is_valid, error = validate_skill_name("") + assert is_valid is False + assert "empty" in error.lower() + + def test_invalid_too_long(self): + """Test invalid name exceeding 64 characters.""" + name = "a" * 65 + is_valid, error = validate_skill_name(name) + assert is_valid is False + assert "64" in error or "65" in error + + def test_invalid_way_too_long(self): + """Test invalid name far exceeding limit.""" + name = "a" * 200 + is_valid, error = validate_skill_name(name) + assert is_valid is False + assert "64" in error or "200" in error + + +class TestNormalizeSkillName: + """Test skill name normalization for creating valid names from any input.""" + + # ========== Basic normalization ========== + + def test_normalize_already_valid(self): + """Test that already valid names remain unchanged.""" + assert normalize_skill_name("my-package") == "my-package" + assert normalize_skill_name("package123") == "package123" + + def test_normalize_uppercase_to_lowercase(self): + """Test uppercase conversion to lowercase.""" + assert normalize_skill_name("MyPackage") == "my-package" + assert normalize_skill_name("MYPACKAGE") == "mypackage" + + def test_normalize_camel_case(self): + """Test camelCase conversion.""" + assert normalize_skill_name("myPackage") == "my-package" + assert normalize_skill_name("myAwesomePackage") == "my-awesome-package" + + def test_normalize_pascal_case(self): + """Test PascalCase conversion.""" + assert normalize_skill_name("MyPackage") == "my-package" + assert normalize_skill_name("MyAwesomePackage") == "my-awesome-package" + + # ========== Separator normalization ========== + + def test_normalize_underscores_to_hyphens(self): + """Test underscores converted to hyphens.""" + assert normalize_skill_name("my_package") == "my-package" + assert normalize_skill_name("my_awesome_package") == "my-awesome-package" + + def test_normalize_spaces_to_hyphens(self): + """Test spaces converted to hyphens.""" + assert normalize_skill_name("my package") == "my-package" + assert normalize_skill_name("my awesome package") == "my-awesome-package" + + def test_normalize_mixed_separators(self): + """Test mixed separators normalized.""" + assert normalize_skill_name("my_awesome package") == "my-awesome-package" + + # ========== Consecutive hyphens ========== + + def test_normalize_removes_consecutive_hyphens(self): + """Test consecutive hyphens are collapsed.""" + assert normalize_skill_name("my--package") == "my-package" + assert normalize_skill_name("my---package") == "my-package" + + def test_normalize_underscores_create_single_hyphen(self): + """Test multiple underscores become single hyphen.""" + assert normalize_skill_name("my___package") == "my-package" + + # ========== Leading/trailing normalization ========== + + def test_normalize_strips_leading_hyphens(self): + """Test leading hyphens are stripped.""" + assert normalize_skill_name("-mypackage") == "mypackage" + assert normalize_skill_name("--mypackage") == "mypackage" + + def test_normalize_strips_trailing_hyphens(self): + """Test trailing hyphens are stripped.""" + assert normalize_skill_name("mypackage-") == "mypackage" + assert normalize_skill_name("mypackage--") == "mypackage" + + def test_normalize_strips_leading_underscores(self): + """Test leading underscores are stripped after conversion.""" + assert normalize_skill_name("_mypackage") == "mypackage" + + def test_normalize_strips_trailing_underscores(self): + """Test trailing underscores are stripped after conversion.""" + assert normalize_skill_name("mypackage_") == "mypackage" + + # ========== Special character removal ========== + + def test_normalize_removes_special_chars(self): + """Test special characters are removed.""" + assert normalize_skill_name("my@package") == "mypackage" + assert normalize_skill_name("my!package#name") == "mypackagename" + + def test_normalize_removes_dots(self): + """Test dots are removed.""" + assert normalize_skill_name("my.package") == "mypackage" + + # ========== Owner/repo format ========== + + def test_normalize_extracts_repo_name(self): + """Test owner/repo format extracts repo name.""" + assert normalize_skill_name("owner/my-package") == "my-package" + assert normalize_skill_name("danielmeppiel/compliance-rules") == "compliance-rules" + + def test_normalize_extracts_and_converts_repo_name(self): + """Test owner/repo format with conversion needed.""" + assert normalize_skill_name("owner/MyPackage") == "my-package" + assert normalize_skill_name("owner/my_package") == "my-package" + + # ========== Truncation ========== + + def test_normalize_truncates_to_64_chars(self): + """Test names are truncated to 64 characters.""" + long_name = "a" * 100 + result = normalize_skill_name(long_name) + assert len(result) == 64 + + def test_normalize_truncation_preserves_content(self): + """Test truncation preserves the start of the name.""" + long_name = "abcdefghij" * 10 # 100 chars + result = normalize_skill_name(long_name) + assert result == "abcdefghij" * 6 + "abcd" # First 64 chars + + # ========== Integration: Normalized names are valid ========== + + def test_normalize_produces_valid_names(self): + """Test that normalized names pass validation.""" + test_inputs = [ + "MyPackage", + "my_awesome_package", + "owner/repo", + "My Package Name", + "package@v1.2.3", + "--leading-hyphens--", + "a" * 100, + "camelCasePackageName", + "UPPERCASE", + ] + + for input_name in test_inputs: + normalized = normalize_skill_name(input_name) + if normalized: # Skip if normalization produces empty string + is_valid, error = validate_skill_name(normalized) + assert is_valid is True, f"normalize_skill_name('{input_name}') = '{normalized}' is invalid: {error}" + + def test_normalize_realistic_package_names(self): + """Test normalization of realistic package names.""" + test_cases = [ + ("danielmeppiel/design-guidelines", "design-guidelines"), + ("ComposioHQ/awesome-claude-skills", "awesome-claude-skills"), + ("github/awesome-copilot", "awesome-copilot"), + ("My_Awesome_Package", "my-awesome-package"), + ("code-review", "code-review"), + ] + + for input_name, expected in test_cases: + result = normalize_skill_name(input_name) + assert result == expected, f"normalize_skill_name('{input_name}') = '{result}', expected '{expected}'" + + +class TestCopySkillToTarget: + """Test the copy_skill_to_target standalone function (T6). + + This tests direct skill copy functionality for native skills + that already have SKILL.md files. + """ + + def setup_method(self): + """Set up test fixtures.""" + self.temp_dir = tempfile.mkdtemp() + self.project_root = Path(self.temp_dir) + self.apm_modules = self.project_root / "apm_modules" + self.apm_modules.mkdir(parents=True) + + def teardown_method(self): + """Clean up after tests.""" + shutil.rmtree(self.temp_dir, ignore_errors=True) + + def _create_package_info( + self, + name: str = "test-pkg", + version: str = "1.0.0", + commit: str = "abc123", + install_path: Path = None, + source: str = None, + description: str = None, + dependency_ref: DependencyReference = None, + pkg_type: PackageContentType = None, + package_type: PackageType = PackageType.CLAUDE_SKILL + ) -> PackageInfo: + """Helper to create PackageInfo objects for tests. + + For native skill tests, package_type defaults to CLAUDE_SKILL since + these packages have SKILL.md and should be installed to .github/skills/. + """ + package = APMPackage( + name=name, + version=version, + package_path=install_path or self.project_root / "package", + source=source or f"github.com/test/{name}", + description=description, + type=pkg_type + ) + resolved_ref = ResolvedReference( + original_ref="main", + ref_type=GitReferenceType.BRANCH, + resolved_commit=commit, + 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(), + dependency_ref=dependency_ref, + package_type=package_type + ) + + # ========== Test T6: Direct copy preserves SKILL.md content exactly ========== + + def test_copy_skill_preserves_skill_md_content_exactly(self): + """Test that direct copy preserves SKILL.md content exactly.""" + # Create a skill package with specific content + skill_source = self.apm_modules / "owner" / "mcp-builder" + skill_source.mkdir(parents=True) + + original_content = """--- +name: mcp-builder +description: Build MCP servers with best practices +version: 1.0.0 +--- + +# MCP Builder + +This skill helps you build **Model Context Protocol** servers. + +## Features + +- TypeScript support +- Python support +- Automatic validation + +## Usage + +Use when building MCP servers or tools. +""" + (skill_source / "SKILL.md").write_text(original_content) + + package_info = self._create_package_info( + name="mcp-builder", + install_path=skill_source, + source="owner/mcp-builder" + ) + + # Copy skill to target + github_path, _ = copy_skill_to_target(package_info, skill_source, self.project_root) + + assert github_path is not None + target_skill_md = github_path / "SKILL.md" + assert target_skill_md.exists() + + # Read copied content + copied_content = target_skill_md.read_text() + + # The body content should be preserved exactly + # (frontmatter will have APM metadata added) + assert "# MCP Builder" in copied_content + assert "This skill helps you build **Model Context Protocol** servers." in copied_content + assert "- TypeScript support" in copied_content + assert "- Python support" in copied_content + assert "- Automatic validation" in copied_content + assert "Use when building MCP servers or tools." in copied_content + + # ========== Test T6: Subdirectories are copied correctly ========== + + def test_copy_skill_copies_scripts_directory(self): + """Test that scripts/ subdirectory is copied correctly.""" + skill_source = self.apm_modules / "owner" / "my-skill" + skill_source.mkdir(parents=True) + + (skill_source / "SKILL.md").write_text("---\nname: my-skill\n---\n# My Skill") + + # Create scripts directory with content + scripts_dir = skill_source / "scripts" + scripts_dir.mkdir() + (scripts_dir / "validate.sh").write_text("#!/bin/bash\necho 'validating...'") + (scripts_dir / "build.py").write_text("#!/usr/bin/env python3\nprint('building...')") + + package_info = self._create_package_info( + name="my-skill", + install_path=skill_source + ) + + github_path, _ = copy_skill_to_target(package_info, skill_source, self.project_root) + + assert github_path is not None + assert (github_path / "scripts").exists() + assert (github_path / "scripts" / "validate.sh").exists() + assert (github_path / "scripts" / "build.py").exists() + + # Verify content preserved + assert "echo 'validating...'" in (github_path / "scripts" / "validate.sh").read_text() + + def test_copy_skill_copies_references_directory(self): + """Test that references/ subdirectory is copied correctly.""" + skill_source = self.apm_modules / "owner" / "my-skill" + skill_source.mkdir(parents=True) + + (skill_source / "SKILL.md").write_text("---\nname: my-skill\n---\n# My Skill") + + # Create references directory with content + refs_dir = skill_source / "references" + refs_dir.mkdir() + (refs_dir / "api-spec.md").write_text("# API Specification\n\nEndpoints...") + (refs_dir / "patterns.md").write_text("# Common Patterns\n\n...") + + package_info = self._create_package_info( + name="my-skill", + install_path=skill_source + ) + + github_path, _ = copy_skill_to_target(package_info, skill_source, self.project_root) + + assert github_path is not None + assert (github_path / "references").exists() + assert (github_path / "references" / "api-spec.md").exists() + assert (github_path / "references" / "patterns.md").exists() + + def test_copy_skill_copies_assets_directory(self): + """Test that assets/ subdirectory is copied correctly.""" + skill_source = self.apm_modules / "owner" / "my-skill" + skill_source.mkdir(parents=True) + + (skill_source / "SKILL.md").write_text("---\nname: my-skill\n---\n# My Skill") + + # Create assets directory with content + assets_dir = skill_source / "assets" + assets_dir.mkdir() + (assets_dir / "template.json").write_text('{"type": "template"}') + (assets_dir / "example.yaml").write_text("name: example\nversion: 1.0") + + package_info = self._create_package_info( + name="my-skill", + install_path=skill_source + ) + + github_path, _ = copy_skill_to_target(package_info, skill_source, self.project_root) + + assert github_path is not None + assert (github_path / "assets").exists() + assert (github_path / "assets" / "template.json").exists() + assert (github_path / "assets" / "example.yaml").exists() + + def test_copy_skill_copies_all_subdirectories(self): + """Test that all skill subdirectories are copied correctly.""" + skill_source = self.apm_modules / "owner" / "complete-skill" + skill_source.mkdir(parents=True) + + (skill_source / "SKILL.md").write_text("---\nname: complete-skill\n---\n# Complete Skill") + + # Create all standard subdirectories + (skill_source / "scripts").mkdir() + (skill_source / "scripts" / "run.sh").write_text("#!/bin/bash") + + (skill_source / "references").mkdir() + (skill_source / "references" / "guide.md").write_text("# Guide") + + (skill_source / "assets").mkdir() + (skill_source / "assets" / "config.json").write_text("{}") + + # Also create a custom subdirectory (should be copied too) + (skill_source / "examples").mkdir() + (skill_source / "examples" / "basic.md").write_text("# Basic Example") + + package_info = self._create_package_info( + name="complete-skill", + install_path=skill_source + ) + + github_path, _ = copy_skill_to_target(package_info, skill_source, self.project_root) + + assert github_path is not None + assert (github_path / "SKILL.md").exists() + assert (github_path / "scripts" / "run.sh").exists() + assert (github_path / "references" / "guide.md").exists() + assert (github_path / "assets" / "config.json").exists() + assert (github_path / "examples" / "basic.md").exists() + + # ========== Test T6: Skill name validation is applied ========== + + def test_copy_skill_validates_skill_name(self): + """Test that skill name is validated when copying.""" + # Create a skill with a valid name + skill_source = self.apm_modules / "owner" / "valid-skill-name" + skill_source.mkdir(parents=True) + (skill_source / "SKILL.md").write_text("---\nname: valid-skill-name\n---\n# Skill") + + package_info = self._create_package_info( + name="valid-skill-name", + install_path=skill_source + ) + + github_path, _ = copy_skill_to_target(package_info, skill_source, self.project_root) + + assert github_path is not None + assert github_path.name == "valid-skill-name" + + def test_copy_skill_normalizes_invalid_skill_name(self): + """Test that invalid skill names are normalized.""" + # Create a skill with an invalid name (uppercase) + skill_source = self.apm_modules / "owner" / "MyInvalidSkillName" + skill_source.mkdir(parents=True) + (skill_source / "SKILL.md").write_text("---\nname: MyInvalidSkillName\n---\n# Skill") + + package_info = self._create_package_info( + name="MyInvalidSkillName", + install_path=skill_source + ) + + github_path, _ = copy_skill_to_target(package_info, skill_source, self.project_root) + + assert github_path is not None + # Name should be normalized to hyphen-case lowercase + assert github_path.name == "my-invalid-skill-name" + + # ========== Test T6: Existing skill is updated on reinstall ========== + + def test_copy_skill_updates_existing_skill(self): + """Test that existing skill is updated on reinstall (overwrite).""" + # Create target skill directory first + skill_dir = self.project_root / ".github" / "skills" / "my-skill" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text("---\nname: my-skill\n---\n# OLD CONTENT") + (skill_dir / "old-file.txt").write_text("This should be removed") + + # Create new source skill + skill_source = self.apm_modules / "owner" / "my-skill" + skill_source.mkdir(parents=True) + (skill_source / "SKILL.md").write_text("---\nname: my-skill\n---\n# NEW CONTENT") + (skill_source / "new-file.txt").write_text("This is new") + + package_info = self._create_package_info( + name="my-skill", + install_path=skill_source + ) + + github_path, _ = copy_skill_to_target(package_info, skill_source, self.project_root) + + assert github_path is not None + assert github_path == skill_dir + + # Verify content is updated + skill_content = (skill_dir / "SKILL.md").read_text() + assert "# NEW CONTENT" in skill_content + assert "# OLD CONTENT" not in skill_content + + # Old file should be removed, new file should exist + assert not (skill_dir / "old-file.txt").exists() + assert (skill_dir / "new-file.txt").exists() + + # ========== Test T6: Packages without SKILL.md are skipped ========== + + def test_copy_skill_skips_packages_without_skill_md(self): + """Test that packages without SKILL.md are skipped.""" + # Create a package without SKILL.md (only has instructions) + pkg_source = self.apm_modules / "owner" / "instructions-only" + pkg_source.mkdir(parents=True) + apm_dir = pkg_source / ".apm" / "instructions" + apm_dir.mkdir(parents=True) + (apm_dir / "coding.instructions.md").write_text("# Coding Standards") + + package_info = self._create_package_info( + name="instructions-only", + install_path=pkg_source + ) + + github_path, claude_path = copy_skill_to_target(package_info, pkg_source, self.project_root) + + # Should return None (skipped) - both paths should be None + assert github_path is None + assert claude_path is None + + # No skill directory should be created + assert not (self.project_root / ".github" / "skills" / "instructions-only").exists() + + # ========== Test T6: Package type routing ========== + + def test_copy_skill_respects_instructions_type(self): + """Test that packages with type='instructions' are skipped.""" + from apm_cli.models.apm_package import PackageContentType + + skill_source = self.apm_modules / "owner" / "my-skill" + skill_source.mkdir(parents=True) + (skill_source / "SKILL.md").write_text("---\nname: my-skill\n---\n# Skill") + + package_info = self._create_package_info( + name="my-skill", + install_path=skill_source, + pkg_type=PackageContentType.INSTRUCTIONS # This type should skip skill install + ) + + github_path, claude_path = copy_skill_to_target(package_info, skill_source, self.project_root) + + # Should return None because type is INSTRUCTIONS - both paths should be None + assert github_path is None + assert claude_path is None + + def test_copy_skill_respects_skill_type(self): + """Test that packages with type='skill' are processed.""" + from apm_cli.models.apm_package import PackageContentType + + skill_source = self.apm_modules / "owner" / "my-skill" + skill_source.mkdir(parents=True) + (skill_source / "SKILL.md").write_text("---\nname: my-skill\n---\n# Skill") + + package_info = self._create_package_info( + name="my-skill", + install_path=skill_source, + pkg_type=PackageContentType.SKILL + ) + + github_path, _ = copy_skill_to_target(package_info, skill_source, self.project_root) + + assert github_path is not None + assert (github_path / "SKILL.md").exists() + + def test_copy_skill_respects_hybrid_type(self): + """Test that packages with type='hybrid' are processed.""" + from apm_cli.models.apm_package import PackageContentType + + skill_source = self.apm_modules / "owner" / "my-skill" + skill_source.mkdir(parents=True) + (skill_source / "SKILL.md").write_text("---\nname: my-skill\n---\n# Skill") + + package_info = self._create_package_info( + name="my-skill", + install_path=skill_source, + pkg_type=PackageContentType.HYBRID + ) + + github_path, _ = copy_skill_to_target(package_info, skill_source, self.project_root) + + assert github_path is not None + assert (github_path / "SKILL.md").exists() + + # ========== Test T6: Creates .github/skills/ if doesn't exist ========== + + def test_copy_skill_creates_github_skills_directory(self): + """Test that .github/skills/ is created if it doesn't exist.""" + # Start with no .github directory + assert not (self.project_root / ".github").exists() + + skill_source = self.apm_modules / "owner" / "my-skill" + skill_source.mkdir(parents=True) + (skill_source / "SKILL.md").write_text("---\nname: my-skill\n---\n# Skill") + + package_info = self._create_package_info( + name="my-skill", + install_path=skill_source + ) + + github_path, _ = copy_skill_to_target(package_info, skill_source, self.project_root) + + assert github_path is not None + assert (self.project_root / ".github" / "skills").exists() + assert (self.project_root / ".github" / "skills" / "my-skill" / "SKILL.md").exists() + + # ========== Test T6: APM metadata is added for orphan detection ========== + + def test_copy_skill_adds_apm_metadata(self): + """Test that APM tracking metadata is added to copied SKILL.md.""" + import frontmatter + + skill_source = self.apm_modules / "owner" / "my-skill" + skill_source.mkdir(parents=True) + (skill_source / "SKILL.md").write_text("---\nname: my-skill\ndescription: Test\n---\n# My Skill") + + package_info = self._create_package_info( + name="my-skill", + version="2.5.0", + commit="xyz789", + install_path=skill_source, + source="owner/my-skill" + ) + + github_path, _ = copy_skill_to_target(package_info, skill_source, self.project_root) + + assert github_path is not None + + # Parse the copied SKILL.md + post = frontmatter.load(github_path / "SKILL.md") + + # Verify APM metadata is present + assert 'metadata' in post.metadata + apm_metadata = post.metadata['metadata'] + assert 'apm_package' in apm_metadata + assert 'apm_version' in apm_metadata + assert apm_metadata['apm_version'] == '2.5.0' + assert 'apm_commit' in apm_metadata + assert apm_metadata['apm_commit'] == 'xyz789' + assert 'apm_installed_at' in apm_metadata + + +class TestNativeSkillIntegration: + """Additional tests for native skill integration via SkillIntegrator._integrate_native_skill (T6). + + These tests verify that packages with existing SKILL.md files are correctly + copied to .github/skills/ and .claude/skills/ directories. + """ + + def setup_method(self): + """Set up test fixtures.""" + self.temp_dir = tempfile.mkdtemp() + self.project_root = Path(self.temp_dir) + self.integrator = SkillIntegrator() + + def teardown_method(self): + """Clean up after tests.""" + shutil.rmtree(self.temp_dir, ignore_errors=True) + + def _create_package_info( + self, + name: str = "test-pkg", + version: str = "1.0.0", + commit: str = "abc123", + install_path: Path = None, + source: str = None, + dependency_ref: DependencyReference = None, + package_type: PackageType = PackageType.CLAUDE_SKILL + ) -> PackageInfo: + """Helper to create PackageInfo objects for tests. + + For native skill tests, package_type defaults to CLAUDE_SKILL since + these packages have SKILL.md and should be installed to .github/skills/. + """ + package = APMPackage( + name=name, + version=version, + package_path=install_path or self.project_root / "package", + source=source or f"github.com/test/{name}" + ) + resolved_ref = ResolvedReference( + original_ref="main", + ref_type=GitReferenceType.BRANCH, + resolved_commit=commit, + 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(), + dependency_ref=dependency_ref, + package_type=package_type + ) + + def test_native_skill_preserves_complete_structure(self): + """Test that native skill integration preserves complete directory structure.""" + # Create a complete skill package + package_dir = self.project_root / "complete-skill" + package_dir.mkdir() + + # Create SKILL.md + (package_dir / "SKILL.md").write_text("""--- +name: complete-skill +description: A complete skill with all subdirectories +--- +# Complete Skill + +Use this skill for comprehensive guidance. +""") + + # Create scripts/ + (package_dir / "scripts").mkdir() + (package_dir / "scripts" / "validate.sh").write_text("#!/bin/bash\necho 'validating'") + + # Create references/ + (package_dir / "references").mkdir() + (package_dir / "references" / "api.md").write_text("# API Reference") + + # Create assets/ + (package_dir / "assets").mkdir() + (package_dir / "assets" / "template.json").write_text('{"key": "value"}') + + package_info = self._create_package_info( + name="complete-skill", + install_path=package_dir + ) + + result = self.integrator.integrate_package_skill(package_info, self.project_root) + + assert result.skill_created is True + assert result.skill_path is not None + + skill_dir = self.project_root / ".github" / "skills" / "complete-skill" + + # Verify all subdirectories are copied + assert (skill_dir / "SKILL.md").exists() + assert (skill_dir / "scripts" / "validate.sh").exists() + assert (skill_dir / "references" / "api.md").exists() + assert (skill_dir / "assets" / "template.json").exists() + + # Verify content preserved + assert "validating" in (skill_dir / "scripts" / "validate.sh").read_text() + assert "API Reference" in (skill_dir / "references" / "api.md").read_text() + + def test_native_skill_normalizes_uppercase_name(self): + """Test that native skill with uppercase folder name is normalized.""" + # Create a skill with uppercase folder name + package_dir = self.project_root / "MyUpperCaseSkill" + package_dir.mkdir() + (package_dir / "SKILL.md").write_text("---\nname: my-skill\n---\n# Skill") + + package_info = self._create_package_info( + name="MyUpperCaseSkill", + install_path=package_dir + ) + + result = self.integrator.integrate_package_skill(package_info, self.project_root) + + assert result.skill_created is True + + # Skill should be installed with normalized name + normalized_skill_dir = self.project_root / ".github" / "skills" / "my-upper-case-skill" + assert normalized_skill_dir.exists() + assert (normalized_skill_dir / "SKILL.md").exists() + + def test_native_skill_files_copied_count(self): + """Test that references_copied accurately counts all copied files.""" + package_dir = self.project_root / "counting-skill" + package_dir.mkdir() + + (package_dir / "SKILL.md").write_text("---\nname: counting-skill\n---\n# Skill") + + (package_dir / "scripts").mkdir() + (package_dir / "scripts" / "a.sh").write_text("a") + (package_dir / "scripts" / "b.sh").write_text("b") + + (package_dir / "references").mkdir() + (package_dir / "references" / "c.md").write_text("c") + + # Total files: SKILL.md + a.sh + b.sh + c.md = 4 + + package_info = self._create_package_info( + name="counting-skill", + install_path=package_dir + ) + + result = self.integrator.integrate_package_skill(package_info, self.project_root) + + assert result.skill_created is True + assert result.references_copied == 4 # All 4 files + + +# ============================================================================= +# T7: Claude Skills Compatibility Copy Tests +# ============================================================================= + +class TestClaudeSkillsCompatibilityCopy: + """Test T7: Claude Skills compatibility copy to .claude/skills/. + + When a skill is installed to .github/skills/, it should also be copied + to .claude/skills/ IF the .claude/ directory already exists. + This ensures Claude Code users get skills while not polluting projects + that don't use Claude. + """ + + def setup_method(self): + """Set up test fixtures.""" + self.temp_dir = tempfile.mkdtemp() + self.project_root = Path(self.temp_dir) + self.apm_modules = self.project_root / "apm_modules" + self.apm_modules.mkdir(parents=True) + self.integrator = SkillIntegrator() + + def teardown_method(self): + """Clean up after tests.""" + shutil.rmtree(self.temp_dir, ignore_errors=True) + + def _create_package_info( + self, + name: str = "test-pkg", + version: str = "1.0.0", + commit: str = "abc123", + install_path: Path = None, + source: str = None, + dependency_ref: DependencyReference = None, + package_type: PackageType = PackageType.CLAUDE_SKILL + ) -> PackageInfo: + """Helper to create PackageInfo objects for tests. + + For skill compatibility tests, package_type defaults to CLAUDE_SKILL since + these packages have SKILL.md and should be installed to .github/skills/. + """ + package = APMPackage( + name=name, + version=version, + package_path=install_path or self.project_root / "package", + source=source or f"github.com/test/{name}" + ) + resolved_ref = ResolvedReference( + original_ref="main", + ref_type=GitReferenceType.BRANCH, + resolved_commit=commit, + 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(), + dependency_ref=dependency_ref, + package_type=package_type + ) + + # ========== Test: Skill copies to .github/skills/ only when .claude/ doesn't exist ========== + + def test_skill_copies_to_github_only_when_no_claude_dir(self): + """Test skill copies to .github/skills/ when .claude/ doesn't exist.""" + # Ensure .claude/ does NOT exist + assert not (self.project_root / ".claude").exists() + + # Create a native skill package + skill_source = self.apm_modules / "owner" / "my-skill" + skill_source.mkdir(parents=True) + (skill_source / "SKILL.md").write_text("---\nname: my-skill\n---\n# My Skill") + + package_info = self._create_package_info( + name="my-skill", + install_path=skill_source + ) + + result = self.integrator.integrate_package_skill(package_info, self.project_root) + + # Should create in .github/skills/ + assert result.skill_created is True + github_skill = self.project_root / ".github" / "skills" / "my-skill" / "SKILL.md" + assert github_skill.exists() + + # Should NOT create .claude/ folder + assert not (self.project_root / ".claude").exists() + + # Should NOT create .claude/skills/ + assert not (self.project_root / ".claude" / "skills").exists() + + # ========== Test: Skill copies to BOTH when .claude/ exists ========== + + def test_skill_copies_to_both_when_claude_exists(self): + """Test skill copies to BOTH .github/skills/ and .claude/skills/ when .claude/ exists.""" + # Create .claude/ directory (simulating a Claude Code project) + (self.project_root / ".claude").mkdir() + + # Create a native skill package + skill_source = self.apm_modules / "owner" / "my-skill" + skill_source.mkdir(parents=True) + (skill_source / "SKILL.md").write_text("---\nname: my-skill\n---\n# My Skill Content") + (skill_source / "references").mkdir() + (skill_source / "references" / "guide.md").write_text("# Guide") + + package_info = self._create_package_info( + name="my-skill", + install_path=skill_source + ) + + result = self.integrator.integrate_package_skill(package_info, self.project_root) + + # Should create in .github/skills/ + assert result.skill_created is True + github_skill_dir = self.project_root / ".github" / "skills" / "my-skill" + assert github_skill_dir.exists() + assert (github_skill_dir / "SKILL.md").exists() + assert (github_skill_dir / "references" / "guide.md").exists() + + # Should ALSO create in .claude/skills/ + claude_skill_dir = self.project_root / ".claude" / "skills" / "my-skill" + assert claude_skill_dir.exists() + assert (claude_skill_dir / "SKILL.md").exists() + assert (claude_skill_dir / "references" / "guide.md").exists() + + # ========== Test: Copies are identical ========== + + def test_copies_are_identical(self): + """Test that .github/skills/ and .claude/skills/ copies are identical.""" + # Create .claude/ directory + (self.project_root / ".claude").mkdir() + + # Create a native skill package with multiple files + skill_source = self.apm_modules / "owner" / "complete-skill" + skill_source.mkdir(parents=True) + + skill_content = """--- +name: complete-skill +description: A complete skill +--- + +# Complete Skill + +Detailed instructions here. +""" + (skill_source / "SKILL.md").write_text(skill_content) + + (skill_source / "scripts").mkdir() + (skill_source / "scripts" / "run.sh").write_text("#!/bin/bash\necho 'running'") + + (skill_source / "references").mkdir() + (skill_source / "references" / "api.md").write_text("# API\n\nEndpoints...") + + (skill_source / "assets").mkdir() + (skill_source / "assets" / "config.json").write_text('{"key": "value"}') + + package_info = self._create_package_info( + name="complete-skill", + install_path=skill_source + ) + + self.integrator.integrate_package_skill(package_info, self.project_root) + + github_skill_dir = self.project_root / ".github" / "skills" / "complete-skill" + claude_skill_dir = self.project_root / ".claude" / "skills" / "complete-skill" + + # Compare all files + github_files = set(f.relative_to(github_skill_dir) for f in github_skill_dir.rglob('*') if f.is_file()) + claude_files = set(f.relative_to(claude_skill_dir) for f in claude_skill_dir.rglob('*') if f.is_file()) + + assert github_files == claude_files, "File structure should be identical" + + # Compare content of each file (except SKILL.md which may have slightly different timestamps) + for rel_path in github_files: + if rel_path.name != "SKILL.md": + github_content = (github_skill_dir / rel_path).read_text() + claude_content = (claude_skill_dir / rel_path).read_text() + assert github_content == claude_content, f"Content of {rel_path} should be identical" + + # SKILL.md should have same body content + github_skill_body = (github_skill_dir / "SKILL.md").read_text() + claude_skill_body = (claude_skill_dir / "SKILL.md").read_text() + assert "# Complete Skill" in github_skill_body + assert "# Complete Skill" in claude_skill_body + assert "Detailed instructions here." in github_skill_body + assert "Detailed instructions here." in claude_skill_body + + # ========== Test: Updates affect both locations ========== + + def test_updates_affect_both_locations(self): + """Test that skill updates affect both .github/skills/ and .claude/skills/.""" + # Create .claude/ directory + (self.project_root / ".claude").mkdir() + + # Create initial skill + skill_source = self.apm_modules / "owner" / "my-skill" + skill_source.mkdir(parents=True) + (skill_source / "SKILL.md").write_text("---\nname: my-skill\n---\n# Version 1") + + package_info_v1 = self._create_package_info( + name="my-skill", + version="1.0.0", + commit="abc123", + install_path=skill_source + ) + + # First install + result1 = self.integrator.integrate_package_skill(package_info_v1, self.project_root) + assert result1.skill_created is True + + # Verify both locations have v1 content + github_content_v1 = (self.project_root / ".github" / "skills" / "my-skill" / "SKILL.md").read_text() + claude_content_v1 = (self.project_root / ".claude" / "skills" / "my-skill" / "SKILL.md").read_text() + assert "# Version 1" in github_content_v1 + assert "# Version 1" in claude_content_v1 + + # Update skill source + (skill_source / "SKILL.md").write_text("---\nname: my-skill\n---\n# Version 2") + + package_info_v2 = self._create_package_info( + name="my-skill", + version="2.0.0", # New version triggers update + commit="def456", + install_path=skill_source + ) + + # Second install (update) + result2 = self.integrator.integrate_package_skill(package_info_v2, self.project_root) + assert result2.skill_updated is True + + # Verify both locations have v2 content + github_content_v2 = (self.project_root / ".github" / "skills" / "my-skill" / "SKILL.md").read_text() + claude_content_v2 = (self.project_root / ".claude" / "skills" / "my-skill" / "SKILL.md").read_text() + assert "# Version 2" in github_content_v2 + assert "# Version 2" in claude_content_v2 + + # ========== Test: .claude/ not created if doesn't exist ========== + + def test_claude_dir_not_created_if_not_exists(self): + """Test that .claude/ directory is NOT created if it doesn't exist.""" + # Ensure .claude/ does NOT exist + assert not (self.project_root / ".claude").exists() + + # Create and install multiple skills + for i in range(3): + skill_source = self.apm_modules / "owner" / f"skill-{i}" + skill_source.mkdir(parents=True) + (skill_source / "SKILL.md").write_text(f"---\nname: skill-{i}\n---\n# Skill {i}") + + package_info = self._create_package_info( + name=f"skill-{i}", + install_path=skill_source + ) + + self.integrator.integrate_package_skill(package_info, self.project_root) + + # .github/skills/ should have all skills + github_skills = self.project_root / ".github" / "skills" + assert github_skills.exists() + assert (github_skills / "skill-0").exists() + assert (github_skills / "skill-1").exists() + assert (github_skills / "skill-2").exists() + + # .claude/ should NOT exist (we never created it) + assert not (self.project_root / ".claude").exists() + + # ========== Test: Generated skills also copy to .claude/ ========== + + def test_generated_skill_copies_to_claude_when_exists(self): + """Test that generated skills (from .apm/ primitives) also copy to .claude/.""" + # Create .claude/ directory + (self.project_root / ".claude").mkdir() + + # Create a package with .apm/ primitives (not a native skill) + package_dir = self.project_root / "my-package" + apm_instructions = package_dir / ".apm" / "instructions" + apm_instructions.mkdir(parents=True) + (apm_instructions / "coding.instructions.md").write_text("# Coding Standards") + + package_info = self._create_package_info( + name="my-package", + install_path=package_dir + ) + + result = self.integrator.integrate_package_skill(package_info, self.project_root) + + assert result.skill_created is True + + # Should exist in .github/skills/ + github_skill = self.project_root / ".github" / "skills" / "my-package" + assert github_skill.exists() + assert (github_skill / "SKILL.md").exists() + + # Should ALSO exist in .claude/skills/ + claude_skill = self.project_root / ".claude" / "skills" / "my-package" + assert claude_skill.exists() + assert (claude_skill / "SKILL.md").exists() + + # ========== Test: copy_skill_to_target returns both paths ========== + + def test_copy_skill_to_target_returns_both_paths_when_claude_exists(self): + """Test that copy_skill_to_target returns both paths when .claude/ exists.""" + # Create .claude/ directory + (self.project_root / ".claude").mkdir() + + skill_source = self.apm_modules / "owner" / "my-skill" + skill_source.mkdir(parents=True) + (skill_source / "SKILL.md").write_text("---\nname: my-skill\n---\n# Skill") + + package_info = self._create_package_info( + name="my-skill", + install_path=skill_source + ) + + github_path, claude_path = copy_skill_to_target(package_info, skill_source, self.project_root) + + assert github_path is not None + assert claude_path is not None + assert github_path == self.project_root / ".github" / "skills" / "my-skill" + assert claude_path == self.project_root / ".claude" / "skills" / "my-skill" + + def test_copy_skill_to_target_returns_none_claude_when_no_claude_dir(self): + """Test that copy_skill_to_target returns None for claude_path when .claude/ doesn't exist.""" + # Ensure .claude/ does NOT exist + assert not (self.project_root / ".claude").exists() + + skill_source = self.apm_modules / "owner" / "my-skill" + skill_source.mkdir(parents=True) + (skill_source / "SKILL.md").write_text("---\nname: my-skill\n---\n# Skill") + + package_info = self._create_package_info( + name="my-skill", + install_path=skill_source + ) + + github_path, claude_path = copy_skill_to_target(package_info, skill_source, self.project_root) + + assert github_path is not None + assert claude_path is None + + # ========== Test: sync_integration cleans both locations ========== + + def test_sync_removes_orphans_from_both_locations(self): + """Test that sync_integration removes orphaned skills from both locations.""" + # Create skill directories in both locations + github_skill = self.project_root / ".github" / "skills" / "orphan-skill" + github_skill.mkdir(parents=True) + (github_skill / "SKILL.md").write_text("""--- +name: orphan-skill +metadata: + apm_package: owner/orphan-skill + apm_version: '1.0.0' +--- +# Orphan Skill +""") + + claude_skill = self.project_root / ".claude" / "skills" / "orphan-skill" + claude_skill.mkdir(parents=True) + (claude_skill / "SKILL.md").write_text("""--- +name: orphan-skill +metadata: + apm_package: owner/orphan-skill + apm_version: '1.0.0' +--- +# Orphan Skill +""") + + # Mock apm_package with no dependencies (orphan) + apm_package = Mock() + apm_package.get_apm_dependencies.return_value = [] + + result = self.integrator.sync_integration(apm_package, self.project_root) + + # Both orphans should be removed + assert result['files_removed'] == 2 + assert not github_skill.exists() + assert not claude_skill.exists() + + def test_sync_keeps_installed_skills_in_both_locations(self): + """Test that sync_integration keeps installed skills in both locations.""" + # Create skill directories in both locations + skill_name = "installed-skill" + canonical_ref = "owner/installed-skill" + + github_skill = self.project_root / ".github" / "skills" / skill_name + github_skill.mkdir(parents=True) + (github_skill / "SKILL.md").write_text(f"""--- +name: {skill_name} +metadata: + apm_package: {canonical_ref} + apm_version: '1.0.0' +--- +# Installed Skill +""") + + claude_skill = self.project_root / ".claude" / "skills" / skill_name + claude_skill.mkdir(parents=True) + (claude_skill / "SKILL.md").write_text(f"""--- +name: {skill_name} +metadata: + apm_package: {canonical_ref} + apm_version: '1.0.0' +--- +# Installed Skill +""") + + # Mock apm_package with this dependency installed + dep_ref = DependencyReference.parse(canonical_ref) + apm_package = Mock() + apm_package.get_apm_dependencies.return_value = [dep_ref] + + result = self.integrator.sync_integration(apm_package, self.project_root) + + # Nothing should be removed + assert result['files_removed'] == 0 + assert github_skill.exists() + assert claude_skill.exists() + + # ========== Test: Only .claude/skills/ cleaned when .claude/ exists ========== + + def test_sync_only_cleans_claude_skills_when_claude_exists(self): + """Test that sync only cleans .claude/skills/ when .claude/ directory exists.""" + # Only .github/ exists, not .claude/ + github_skill = self.project_root / ".github" / "skills" / "orphan-skill" + github_skill.mkdir(parents=True) + (github_skill / "SKILL.md").write_text("""--- +name: orphan-skill +metadata: + apm_package: owner/orphan-skill +--- +# Orphan Skill +""") + + apm_package = Mock() + apm_package.get_apm_dependencies.return_value = [] + + result = self.integrator.sync_integration(apm_package, self.project_root) + + # Only the github orphan should be removed (claude doesn't exist) + assert result['files_removed'] == 1 + assert not github_skill.exists() + assert not (self.project_root / ".claude").exists() + + # ========== Test: APM metadata added to both copies ========== + + def test_apm_metadata_added_to_both_copies(self): + """Test that APM metadata is added to SKILL.md in both locations.""" + import frontmatter + + # Create .claude/ directory + (self.project_root / ".claude").mkdir() + + skill_source = self.apm_modules / "owner" / "my-skill" + skill_source.mkdir(parents=True) + (skill_source / "SKILL.md").write_text("---\nname: my-skill\ndescription: Test\n---\n# My Skill") + + package_info = self._create_package_info( + name="my-skill", + version="2.0.0", + commit="xyz789", + install_path=skill_source, + source="owner/my-skill" + ) + + self.integrator.integrate_package_skill(package_info, self.project_root) + + # Check .github/skills/ + github_post = frontmatter.load(self.project_root / ".github" / "skills" / "my-skill" / "SKILL.md") + assert 'metadata' in github_post.metadata + assert github_post.metadata['metadata']['apm_version'] == '2.0.0' + assert github_post.metadata['metadata']['apm_commit'] == 'xyz789' + + # Check .claude/skills/ + claude_post = frontmatter.load(self.project_root / ".claude" / "skills" / "my-skill" / "SKILL.md") + assert 'metadata' in claude_post.metadata + assert claude_post.metadata['metadata']['apm_version'] == '2.0.0' + assert claude_post.metadata['metadata']['apm_commit'] == 'xyz789' + + # ========== T12: Additional orphan cleanup tests ========== + + def test_sync_preserves_user_created_skills_without_apm_metadata(self): + """Test that sync does NOT remove user-created skills without APM metadata. + + User-created skill directories (without apm_package in metadata) should + never be removed during sync. This prevents data loss of manually created skills. + """ + # Create a user-created skill in .github/skills/ (no APM metadata) + user_skill = self.project_root / ".github" / "skills" / "user-created-skill" + user_skill.mkdir(parents=True) + (user_skill / "SKILL.md").write_text("""--- +name: user-created-skill +description: A skill I created manually +--- +# My Custom Skill + +This is a skill I created by hand, not via APM. +""") + + # Create a user-created skill in .claude/skills/ (no APM metadata) + (self.project_root / ".claude").mkdir() + claude_user_skill = self.project_root / ".claude" / "skills" / "my-workflow" + claude_user_skill.mkdir(parents=True) + (claude_user_skill / "SKILL.md").write_text("""--- +name: my-workflow +description: Custom workflow +--- +# Workflow +""") + + # Run sync with no dependencies (simulates `apm prune`) + apm_package = Mock() + apm_package.get_apm_dependencies.return_value = [] + + result = self.integrator.sync_integration(apm_package, self.project_root) + + # User skills should NOT be removed (no apm_package metadata) + assert result['files_removed'] == 0 + assert user_skill.exists() + assert claude_user_skill.exists() + + def test_sync_skips_skill_dirs_without_skill_md(self): + """Test that sync gracefully handles skill directories without SKILL.md. + + Skill directories without a SKILL.md file should be skipped, not removed. + This can happen with corrupted installs or partial cleanups. + """ + # Create a skill directory without SKILL.md + empty_skill = self.project_root / ".github" / "skills" / "empty-skill" + empty_skill.mkdir(parents=True) + # Just add some other file + (empty_skill / "README.md").write_text("# Some file") + + apm_package = Mock() + apm_package.get_apm_dependencies.return_value = [] + + result = self.integrator.sync_integration(apm_package, self.project_root) + + # Should not be removed (no SKILL.md to check metadata) + assert result['files_removed'] == 0 + assert empty_skill.exists() + + def test_sync_handles_malformed_skill_md_gracefully(self): + """Test that sync handles SKILL.md with malformed frontmatter gracefully. + + If a SKILL.md has invalid YAML frontmatter, it is treated as a user-created + skill (no APM metadata found) and is NOT removed. This is the safe behavior + to prevent accidental data loss. + """ + # Create a skill with malformed frontmatter + malformed_skill = self.project_root / ".github" / "skills" / "malformed" + malformed_skill.mkdir(parents=True) + (malformed_skill / "SKILL.md").write_text("""--- +invalid yaml: [this is broken + no closing bracket +--- +# Content +""") + + apm_package = Mock() + apm_package.get_apm_dependencies.return_value = [] + + result = self.integrator.sync_integration(apm_package, self.project_root) + + # Skill should NOT be removed (treated as no APM metadata = user-created) + assert result['files_removed'] == 0 + assert malformed_skill.exists() + + def test_sync_removes_orphans_only_from_github_when_no_claude(self): + """Test cleanup works correctly when .claude/ directory doesn't exist. + + When .claude/ doesn't exist, only .github/skills/ should be cleaned. + """ + # Ensure .claude/ does NOT exist + assert not (self.project_root / ".claude").exists() + + # Create an APM-managed orphan skill in .github/skills/ + orphan_skill = self.project_root / ".github" / "skills" / "orphan" + orphan_skill.mkdir(parents=True) + (orphan_skill / "SKILL.md").write_text("""--- +name: orphan +metadata: + apm_package: owner/orphan-pkg + apm_version: '1.0.0' +--- +# Orphan Skill +""") + + apm_package = Mock() + apm_package.get_apm_dependencies.return_value = [] + + result = self.integrator.sync_integration(apm_package, self.project_root) + + # Only github orphan should be removed + assert result['files_removed'] == 1 + assert not orphan_skill.exists() + + def test_sync_aggregates_stats_from_both_locations(self): + """Test that sync correctly aggregates removal stats from both locations. + + When orphans exist in both .github/skills/ and .claude/skills/, + the stats should reflect total removals from both locations. + """ + # Create .claude/ directory + (self.project_root / ".claude").mkdir() + + # Create orphan in .github/skills/ + github_orphan = self.project_root / ".github" / "skills" / "orphan-a" + github_orphan.mkdir(parents=True) + (github_orphan / "SKILL.md").write_text("""--- +name: orphan-a +metadata: + apm_package: owner/orphan-a +--- +# Orphan A +""") + + # Create different orphan in .claude/skills/ + claude_orphan = self.project_root / ".claude" / "skills" / "orphan-b" + claude_orphan.mkdir(parents=True) + (claude_orphan / "SKILL.md").write_text("""--- +name: orphan-b +metadata: + apm_package: owner/orphan-b +--- +# Orphan B +""") + + apm_package = Mock() + apm_package.get_apm_dependencies.return_value = [] + + result = self.integrator.sync_integration(apm_package, self.project_root) + + # Both orphans should be removed (1 from each location) + assert result['files_removed'] == 2 + assert not github_orphan.exists() + assert not claude_orphan.exists() diff --git a/tests/unit/integration/test_skill_transformer.py b/tests/unit/integration/test_skill_transformer.py index de8347987..fdd226e0d 100644 --- a/tests/unit/integration/test_skill_transformer.py +++ b/tests/unit/integration/test_skill_transformer.py @@ -1,4 +1,10 @@ -"""Tests for skill transformer functionality (SKILL.md → .agent.md conversion).""" +"""Tests for skill transformer functionality. + +Note: The SkillTransformer class is used internally by SkillIntegrator for +skill name normalization and formatting. Skills are NO LONGER transformed +to .agent.md files - they go directly to .github/skills/ as native skills. +See skill-strategy.md for architectural rationale (T5). +""" import tempfile import shutil @@ -49,7 +55,12 @@ def test_strips_leading_trailing_hyphens(self): class TestSkillTransformer: - """Test SkillTransformer class.""" + """Test SkillTransformer class. + + Note: The SkillTransformer is kept for backwards compatibility and internal use. + The transform_to_agent() method is deprecated but still functional for testing + purposes. In production, skills go directly to .github/skills/ via SkillIntegrator. + """ def setup_method(self): """Set up test fixtures.""" @@ -176,61 +187,9 @@ def test_transform_complex_skill_name(self): assert result.name == "my-awesome-skill-v2.agent.md" -class TestAgentIntegratorSkillSupport: - """Test AgentIntegrator's skill handling.""" - - def setup_method(self): - """Set up test fixtures.""" - self.temp_dir = tempfile.mkdtemp() - self.project_root = Path(self.temp_dir) - self.package_dir = self.project_root / "apm_modules" / "test-package" - self.package_dir.mkdir(parents=True) - - def teardown_method(self): - """Clean up after tests.""" - shutil.rmtree(self.temp_dir, ignore_errors=True) - - def test_find_skill_file_when_exists(self): - """Test finding SKILL.md when it exists.""" - from apm_cli.integration.agent_integrator import AgentIntegrator - - # Create SKILL.md - skill_file = self.package_dir / "SKILL.md" - skill_file.write_text("---\nname: Test\n---\n# Content") - - integrator = AgentIntegrator() - result = integrator.find_skill_file(self.package_dir) - - assert result is not None - assert result == skill_file - - def test_find_skill_file_when_not_exists(self): - """Test finding SKILL.md when it doesn't exist.""" - from apm_cli.integration.agent_integrator import AgentIntegrator - - integrator = AgentIntegrator() - result = integrator.find_skill_file(self.package_dir) - - assert result is None - - def test_find_skill_file_case_sensitive(self): - """Test SKILL.md detection on case-insensitive filesystems. - - Note: On macOS/Windows with case-insensitive filesystems, skill.md - will match SKILL.md. On Linux (case-sensitive), it won't. - This test documents the expected behavior. - """ - from apm_cli.integration.agent_integrator import AgentIntegrator - - # Create lowercase skill.md - skill_file = self.package_dir / "skill.md" - skill_file.write_text("---\nname: Test\n---\n# Content") - - integrator = AgentIntegrator() - result = integrator.find_skill_file(self.package_dir) - - # On case-insensitive filesystems (macOS, Windows), this will match - # On case-sensitive filesystems (Linux), it won't - # We just verify the function doesn't crash - if result is not None: - assert result.name.upper() == "SKILL.MD" +# NOTE: TestAgentIntegratorSkillSupport class has been REMOVED as part of T5. +# +# The find_skill_file() method was removed from AgentIntegrator because: +# - Skills are NO LONGER transformed to .agent.md files +# - Skills now go directly to .github/skills/ via SkillIntegrator +# - See skill-strategy.md for the full architectural rationale