diff --git a/CHANGELOG.md b/CHANGELOG.md index 982989477..1ee9872d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- HYBRID packages (apm.yml + SKILL.md without `.apm/`) now install as skill bundles per agentskills.io semantics; previously `apm install` rejected this layout in the validator and emitted no inline error, producing a perceived hang. (#946) +- CLAUDE_SKILL packages (SKILL.md only) that ship co-located resource directories (`agents/`, `assets/`, `scripts/`) now route through the skill-bundle install path. (#946) +- Direct-dependency integration failures now print a `[x]` line immediately and exit 1 instead of failing silently. (#946) +- HYBRID metadata model: `apm.yml` description and `SKILL.md` description are independent fields with different consumers and are no longer merged; `apm pack` warns when `apm.yml` is missing `description` on a HYBRID package. (#946) +- Symlink hardening extended to all three `skill_integrator` copytree paths. (#946) - `apm install` (user scope): `init_link_resolver` now scopes `discover_primitives` to `~/.apm/` instead of `~/`, preventing recursive-glob across the entire home directory. Fixes #830 (#850) - Audit blindness for local `.apm/` content -- `apm audit --ci` now detects drift, missing files, and content tampering on locally-authored files (not just installed packages). (#887) - Packer leak risk: local-content fields (`local_deployed_files`, `local_deployed_file_hashes`) are now stripped from bundled lockfiles, preventing phantom self-entries on unpack. (#887) diff --git a/docs/src/content/docs/getting-started/first-package.md b/docs/src/content/docs/getting-started/first-package.md index aa58b25c4..88711db02 100644 --- a/docs/src/content/docs/getting-started/first-package.md +++ b/docs/src/content/docs/getting-started/first-package.md @@ -267,6 +267,25 @@ audit while you author; pack produces the plugin bundle when you ship. For the full reference, see the [Pack & Distribute guide](/apm/guides/pack-distribute/) and the [Plugin authoring guide](/apm/guides/plugins/). +## Choosing a package layout + +APM recognizes three layouts. Pick the one that matches what you are shipping: + +- **One skill** -- put `SKILL.md` at the repo root, with optional + `agents/`, `assets/`, or `scripts/` directories alongside it. Add + `apm.yml` if you need dependency management (this is a HYBRID package). + APM installs the whole directory as a single skill bundle. + +- **Multiple primitives** -- use the `.apm/` directory with `skills/`, + `agents/`, `instructions/` subdirectories (the layout used in this guide). + APM hoists each primitive into the consumer's runtime dirs individually. + +- **Claude plugin** -- if you already have a `plugin.json`, APM can consume + it directly without restructuring. + +For the full comparison and metadata precedence rules, see +[Package Types](../../reference/package-types/). + ## Next steps - [Anatomy of an APM Package](/apm/introduction/anatomy-of-an-apm-package/) diff --git a/docs/src/content/docs/reference/examples.md b/docs/src/content/docs/reference/examples.md index 43faa6d69..52421419d 100644 --- a/docs/src/content/docs/reference/examples.md +++ b/docs/src/content/docs/reference/examples.md @@ -1,7 +1,7 @@ --- title: "Examples" sidebar: - order: 4 + order: 5 --- This guide showcases real-world APM workflows, from simple automation to enterprise-scale AI development patterns. Learn through practical examples that demonstrate the power of structured AI workflows. diff --git a/docs/src/content/docs/reference/experimental.md b/docs/src/content/docs/reference/experimental.md index 8b261b8c2..132441ff8 100644 --- a/docs/src/content/docs/reference/experimental.md +++ b/docs/src/content/docs/reference/experimental.md @@ -2,7 +2,7 @@ title: "apm experimental" description: "Manage opt-in experimental feature flags. Evaluate new or changing behaviour without affecting APM defaults." sidebar: - order: 5 + order: 6 label: "Experimental Flags" --- diff --git a/docs/src/content/docs/reference/package-types.md b/docs/src/content/docs/reference/package-types.md new file mode 100644 index 000000000..058e0cef9 --- /dev/null +++ b/docs/src/content/docs/reference/package-types.md @@ -0,0 +1,125 @@ +--- +title: "Package Types" +sidebar: + order: 4 +--- + +APM supports three package layouts, each with distinct install semantics. +Pick the layout that matches the author's intent -- APM preserves it. + +## Layout summary + +| Root signal | Author intent | Install semantic | +|---|---|---| +| `.apm/` (with or without apm.yml) | "I have N independent primitives" | Hoist each primitive into the target's runtime dirs | +| `SKILL.md` (alone or with apm.yml -- HYBRID) | "I am one skill bundle" | Copy the whole bundle to `/skills//` | +| `plugin.json` / `.claude-plugin/` | Claude plugin collection | Dissect via plugin artifact mapping | + +## APM package (`.apm/` directory) + +The classic APM layout. Primitives live under `.apm/` in typed subdirectories. +`apm install` hoists each primitive into the consumer's runtime directories +individually. + +``` +my-package/ ++-- apm.yml ++-- .apm/ + +-- skills/ + | +-- pr-description/SKILL.md + +-- agents/ + | +-- reviewer.agent.md + +-- instructions/ + +-- team-standards.instructions.md +``` + +**What gets installed:** each skill, agent, and instruction is copied to its +corresponding runtime directory (e.g. `.github/skills/`, `.github/agents/`). + +**When to choose:** you are shipping multiple independent primitives that +consumers may override or extend individually. + +## Skill bundle (`SKILL.md` at root) + +A single skill with co-located resources. The presence of `SKILL.md` at the +package root tells APM: "this entire directory is one skill -- install it as +a unit." + +An optional `apm.yml` alongside `SKILL.md` makes this a **HYBRID** package. +APM still installs it as a skill bundle, but gains dependency resolution, +version metadata, and script support from the manifest. + +``` +code-review-skill/ ++-- SKILL.md ++-- agents/ +| +-- reviewer.agent.md ++-- assets/ +| +-- checklist.md ++-- scripts/ +| +-- lint-check.sh ++-- apm.yml # optional -- enables dependencies and scripts +``` + +**What gets installed:** the entire directory tree is copied to +`/skills//`, preserving internal structure. + +**When to choose:** you are shipping one cohesive skill that bundles its own +agents, assets, or scripts. The skill's internal layout is part of its +contract -- APM will not rearrange it. + +### Metadata model (HYBRID packages) + +`apm.yml` and `SKILL.md` each own their `description` field +**independently** -- APM never merges or backfills one from the other. +The two strings serve different consumers: + +- `apm.yml.description` is a short human-facing tagline rendered by + `apm view`, `apm search`, `apm deps list`, and registry/marketplace + listings. +- `SKILL.md` `description` (frontmatter) is the agent-runtime + invocation matcher consumed by Claude, Copilot, and other runtimes + per the agentskills.io spec. APM copies `SKILL.md` byte-for-byte + into `/skills//` and never reads or mutates this + field. + +Other apm.yml fields (`name`, `version`, `license`, `dependencies`, +`scripts`) are owned exclusively by `apm.yml` -- there is no +SKILL.md-side equivalent and nothing to merge. `allowed-tools` lives +exclusively in `SKILL.md` frontmatter and is consumed by the agent +runtime. + +When you ship a HYBRID package, populate both descriptions +independently: keep `apm.yml.description` to a short tagline (under +~80 characters) and write `SKILL.md` in whatever length and tone the +agent runtime expects. `apm pack` warns when `apm.yml.description` is +missing so the human-facing surfaces do not degrade silently while +the agent runtime keeps working. + +## Plugin collection (`plugin.json`) + +A Claude-native plugin layout. APM dissects the plugin artifacts and maps +them into runtime directories. + +``` +my-plugin/ ++-- plugin.json ++-- agents/ +| +-- helper.agent.md ++-- skills/ + +-- search/SKILL.md +``` + +**What gets installed:** each artifact listed in `plugin.json` is mapped to +the appropriate runtime directory via `_map_plugin_artifacts`. + +**When to choose:** you already have a Claude plugin and want APM to +consume it without restructuring. + +## See also + +- [Your First Package](../../getting-started/first-package/) -- hands-on + walkthrough for scaffolding and publishing. +- [CLI Commands](../cli-commands/) -- `apm install`, `apm pack`, and all + options. +- [Manifest Schema](../manifest-schema/) -- full `apm.yml` field reference. diff --git a/packages/apm-guide/.apm/skills/apm-usage/commands.md b/packages/apm-guide/.apm/skills/apm-usage/commands.md index c5a3e6a99..32198b1e2 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/commands.md +++ b/packages/apm-guide/.apm/skills/apm-usage/commands.md @@ -10,7 +10,7 @@ | Command | Purpose | Key flags | |---------|---------|-----------| -| `apm install [PKGS...]` | Install packages | `--update` refresh refs, `--force` overwrite, `--dry-run`, `--verbose`, `--only [apm\|mcp]`, `--target` (comma-separated), `--dev`, `-g` global, `--trust-transitive-mcp`, `--parallel-downloads N`, `--allow-insecure`, `--allow-insecure-host HOSTNAME`, `--mcp NAME` add MCP entry, `--transport`, `--url`, `--env KEY=VAL`, `--header KEY=VAL`, `--mcp-version`, `--registry URL` custom MCP registry | +| `apm install [PKGS...]` | Install APM and MCP dependencies (supports APM packages, Claude skills (SKILL.md), and plugin collections (plugin.json)) | `--update` refresh refs, `--force` overwrite, `--dry-run`, `--verbose`, `--only [apm\|mcp]`, `--target` (comma-separated), `--dev`, `-g` global, `--trust-transitive-mcp`, `--parallel-downloads N`, `--allow-insecure`, `--allow-insecure-host HOSTNAME`, `--mcp NAME` add MCP entry, `--transport`, `--url`, `--env KEY=VAL`, `--header KEY=VAL`, `--mcp-version`, `--registry URL` custom MCP registry | | `apm uninstall PKGS...` | Remove packages | `--dry-run`, `-g` global | | `apm prune` | Remove orphaned packages | `--dry-run` | | `apm deps list` | List installed packages | `-g` global, `--all` both scopes, `--insecure` | diff --git a/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md b/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md index 546d643d9..49cb62fa2 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md +++ b/packages/apm-guide/.apm/skills/apm-usage/package-authoring.md @@ -1,6 +1,40 @@ # Package Authoring -## Package directory structure +## Supported package layouts + +APM recognizes three layouts. The shape of the package root tells APM +how to install it: + +| Root signal | Author intent | Install semantic | +|---|---|---| +| `.apm/` (with or without apm.yml) | Multiple independent primitives | Hoist each primitive into the consumer runtime dirs | +| `SKILL.md` (alone, or with apm.yml = HYBRID) | One skill bundle | Copy whole tree to `/skills//` | +| `plugin.json` / `.claude-plugin/` | Claude plugin collection | Dissect via plugin artifact mapping | + +The HYBRID layout (apm.yml + SKILL.md) is a single skill bundle that +also uses APM dependency resolution. APM installs it as a skill -- it +does NOT dissect the bundle into top-level primitives. Co-located +subdirectories like `agents/`, `assets/`, `scripts/` are bundle +resources, not standalone primitives. + +In a HYBRID package, `apm.yml` and `SKILL.md` each own their +`description` field **independently** -- APM never merges or +backfills one from the other: +- `apm.yml.description` is a short human-facing tagline rendered by + `apm view`, `apm search`, `apm deps list`, and registry listings. +- `SKILL.md` `description` (frontmatter) is the agent-runtime + invocation matcher (per agentskills.io). APM copies `SKILL.md` + byte-for-byte and never reads or mutates this field. +- `allowed-tools` lives exclusively in `SKILL.md` frontmatter; there + is no apm.yml-side equivalent. +- `name`, `version`, `license`, `dependencies`, `scripts` live + exclusively in `apm.yml`. + +Populate both descriptions when you ship a HYBRID package. `apm pack` +warns when `apm.yml.description` is missing so listings do not +degrade silently while the agent runtime keeps working. + +## Package directory structure (APM layout) ``` my-package/ diff --git a/src/apm_cli/bundle/packer.py b/src/apm_cli/bundle/packer.py index 9f279d993..f871b71cf 100644 --- a/src/apm_cli/bundle/packer.py +++ b/src/apm_cli/bundle/packer.py @@ -80,12 +80,38 @@ def pack_bundle( # 2. Read apm.yml for name / version / config target apm_yml_path = project_root / "apm.yml" + skill_md_path = project_root / "SKILL.md" + is_hybrid_root = apm_yml_path.exists() and skill_md_path.exists() try: package = APMPackage.from_apm_yml(apm_yml_path) pkg_name = package.name pkg_version = package.version or "0.0.0" config_target = package.target + # HYBRID author guard: apm.yml.description and SKILL.md + # description serve different consumers (human-facing CLI/search + # vs. agent-runtime invocation matcher) and are NOT merged. If + # the author shipped a SKILL.md description but left + # apm.yml.description blank, the human-facing surfaces (apm view, + # apm search, marketplace listings) will degrade silently while + # Claude/Copilot still invoke the skill correctly. Warn loudly + # at pack time -- this is the publish gate for the AUTHOR. + if is_hybrid_root and not package.description and logger: + try: + import frontmatter as _frontmatter + with open(skill_md_path, "r", encoding="utf-8") as _f: + _skill_post = _frontmatter.load(_f) + _skill_desc = _skill_post.metadata.get("description") + except Exception: + _skill_desc = None + if _skill_desc: + logger.warning( + "apm.yml is missing 'description'. SKILL.md has its own " + "description, but that is for agent invocation -- not " + "for 'apm view' or search. Add a short tagline to " + "apm.yml: description: \"One-line human summary\"" + ) + # Guard: reject local-path dependencies (non-portable) for dep_ref in package.get_apm_dependencies(): if dep_ref.is_local: diff --git a/src/apm_cli/commands/deps/_utils.py b/src/apm_cli/commands/deps/_utils.py index 403c832ee..0832eb6f1 100644 --- a/src/apm_cli/commands/deps/_utils.py +++ b/src/apm_cli/commands/deps/_utils.py @@ -183,10 +183,25 @@ def _get_detailed_package_info(package_path: Path) -> Dict[str, Any]: package = APMPackage.from_apm_yml(apm_yml_path) context_count, workflow_count = _count_package_files(package_path) primitives = _count_primitives(package_path) + # HYBRID-aware description rendering: when apm.yml omits its + # tagline but a SKILL.md sits alongside, surface the empty + # apm.yml.description as `--` plus an inline annotation. The + # SKILL.md description is intentionally NOT borrowed -- it is + # an agent invocation matcher, not a human tagline. + is_hybrid = (package_path / "SKILL.md").exists() + if package.description: + desc = package.description + elif is_hybrid: + desc = ( + "-- (set 'description' in apm.yml; SKILL.md " + "description is for agent runtime)" + ) + else: + desc = 'No description' return { 'name': package.name, 'version': package.version or 'unknown', - 'description': package.description or 'No description', + 'description': desc, 'author': package.author or 'Unknown', 'source': package.source or 'local', 'install_path': str(package_path.resolve()), diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index c56aa454c..61de53794 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -60,7 +60,7 @@ _has_local_apm_content, _project_has_root_primitives, ) -from apm_cli.install.errors import PolicyViolationError +from apm_cli.install.errors import DirectDependencyError, PolicyViolationError from apm_cli.install.insecure_policy import ( _InsecureDependencyInfo, _allow_insecure_host_callback, @@ -912,7 +912,7 @@ def _run_mcp_install( @click.command( - help="Install APM and MCP dependencies (auto-creates apm.yml; use --allow-insecure for http:// packages)" + help="Install APM and MCP dependencies (supports APM packages, Claude skills (SKILL.md), and plugin collections (plugin.json); auto-creates apm.yml; use --allow-insecure for http:// packages)" ) @click.argument("packages", nargs=-1) @click.option("--runtime", help="Target specific runtime only (copilot, codex, vscode)") @@ -1597,6 +1597,10 @@ def install(ctx, packages, runtime, exclude, only, update, dry_run, force, verbo except InsecureDependencyPolicyError: _maybe_rollback_manifest(_snapshot_manifest_path, _manifest_snapshot, logger) sys.exit(1) + except DirectDependencyError as e: + _maybe_rollback_manifest(_snapshot_manifest_path, _manifest_snapshot, logger) + logger.error(str(e)) + sys.exit(1) except click.UsageError: # Conflict matrix / argv parser raises UsageError -- let Click # render with exit code 2 and the standard "Usage: ..." prefix. diff --git a/src/apm_cli/deps/package_validator.py b/src/apm_cli/deps/package_validator.py index b06b7faeb..24f02077a 100644 --- a/src/apm_cli/deps/package_validator.py +++ b/src/apm_cli/deps/package_validator.py @@ -66,46 +66,54 @@ def validate_package_structure(self, package_path: Path) -> ValidationResult: result.add_error(f"Invalid apm.yml: {e}") return result - # Check for .apm directory + # Check for .apm directory -- only mandatory for APM_PACKAGE layout. + # HYBRID and CLAUDE_SKILL packages may ship without .apm/. + from ..models.validation import detect_package_type, PackageType + + pkg_type, _ = detect_package_type(package_path) apm_dir = package_path / ".apm" - if not apm_dir.exists(): - result.add_error("Missing required directory: .apm/") - return result - - if not apm_dir.is_dir(): - result.add_error(".apm must be a directory") - return result - - # Check for primitive content - primitive_types = ['instructions', 'chatmodes', 'contexts', 'prompts'] - has_primitives = False - - for primitive_type in primitive_types: - primitive_dir = apm_dir / primitive_type - if primitive_dir.exists() and primitive_dir.is_dir(): - md_files = list(primitive_dir.glob("*.md")) - if md_files: + if pkg_type in (PackageType.APM_PACKAGE, PackageType.INVALID) or pkg_type is None: + if not apm_dir.exists(): + result.add_error("Missing required directory: .apm/") + return result + + if not apm_dir.is_dir(): + result.add_error(".apm must be a directory") + return result + + # Check for primitive content -- only meaningful when .apm/ exists. + # HYBRID and CLAUDE_SKILL layouts may ship without .apm/, so the + # "no primitives" warning would be misleading for those shapes. + if apm_dir.exists() and apm_dir.is_dir(): + primitive_types = ['instructions', 'chatmodes', 'contexts', 'prompts'] + has_primitives = False + + for primitive_type in primitive_types: + primitive_dir = apm_dir / primitive_type + if primitive_dir.exists() and primitive_dir.is_dir(): + md_files = list(primitive_dir.glob("*.md")) + if md_files: + has_primitives = True + # Validate each primitive file + for md_file in md_files: + self._validate_primitive_file(md_file, result) + + # Check for hooks (JSON files, not markdown) + hooks_dir = apm_dir / "hooks" + if hooks_dir.exists() and hooks_dir.is_dir(): + json_files = list(hooks_dir.glob("*.json")) + if json_files: has_primitives = True - # Validate each primitive file - for md_file in md_files: - self._validate_primitive_file(md_file, result) - # Check for hooks (JSON files, not markdown) - hooks_dir = apm_dir / "hooks" - if hooks_dir.exists() and hooks_dir.is_dir(): - json_files = list(hooks_dir.glob("*.json")) - if json_files: - has_primitives = True - - # Also check hooks/ at package root (Claude-native convention) - hooks_root_dir = package_path / "hooks" - if hooks_root_dir.exists() and hooks_root_dir.is_dir(): - json_files = list(hooks_root_dir.glob("*.json")) - if json_files: - has_primitives = True - - if not has_primitives: - result.add_warning("No primitive files found in .apm/ directory") + # Also check hooks/ at package root (Claude-native convention) + hooks_root_dir = package_path / "hooks" + if hooks_root_dir.exists() and hooks_root_dir.is_dir(): + json_files = list(hooks_root_dir.glob("*.json")) + if json_files: + has_primitives = True + + if not has_primitives: + result.add_warning("No primitive files found in .apm/ directory") return result diff --git a/src/apm_cli/install/context.py b/src/apm_cli/install/context.py index 46c1869fa..4a587ed28 100644 --- a/src/apm_cli/install/context.py +++ b/src/apm_cli/install/context.py @@ -60,6 +60,12 @@ class InstallContext: # ------------------------------------------------------------------ # Resolve phase outputs # ------------------------------------------------------------------ + # Direct dependencies declared in apm.yml (regular + dev), NOT the + # full transitive closure. Transitive deps are discovered later by + # the resolver and recorded on `deps_to_install` / + # `dependency_graph`. Treat `all_apm_deps` as "what the project + # author wrote" -- iterate `deps_to_install` for the full set of + # packages that will be installed. all_apm_deps: List[Any] = field(default_factory=list) # resolve root_has_local_primitives: bool = False # resolve deps_to_install: List[Any] = field(default_factory=list) # resolve @@ -109,6 +115,7 @@ class InstallContext: total_commands_integrated: int = 0 # integrate total_hooks_integrated: int = 0 # integrate total_links_resolved: int = 0 # integrate + direct_dep_failed: bool = False # integrate -- set when any direct dep fails # ------------------------------------------------------------------ # policy_gate diff --git a/src/apm_cli/install/errors.py b/src/apm_cli/install/errors.py index 24aaf6642..95d439ca4 100644 --- a/src/apm_cli/install/errors.py +++ b/src/apm_cli/install/errors.py @@ -22,6 +22,16 @@ from apm_cli.policy.models import CIAuditResult +class DirectDependencyError(RuntimeError): + """Raised when one or more direct dependencies fail validation or integration. + + Bypasses the broad ``except Exception`` wrapper in ``pipeline.py`` so the + original message reaches ``commands/install.py`` without being double-wrapped + as ``"Failed to resolve APM dependencies: ..."`` (same pattern as + :class:`PolicyViolationError`). + """ + + class PolicyViolationError(RuntimeError): """Raised when org-policy enforcement halts an install. diff --git a/src/apm_cli/install/phases/integrate.py b/src/apm_cli/install/phases/integrate.py index 7ba369746..efadd2d61 100644 --- a/src/apm_cli/install/phases/integrate.py +++ b/src/apm_cli/install/phases/integrate.py @@ -303,6 +303,12 @@ def run(ctx: "InstallContext") -> None: deps_to_install = ctx.deps_to_install apm_modules_dir = ctx.apm_modules_dir + # Direct dep keys: used to distinguish direct vs transitive failures + # so direct failures can be surfaced immediately. + direct_dep_keys = builtins.set( + dep.get_unique_key() for dep in ctx.all_apm_deps + ) + # Int counters (written back to ctx at end of function) installed_count = ctx.installed_count unpinned_count = ctx.unpinned_count @@ -367,6 +373,25 @@ def run(ctx: "InstallContext") -> None: deltas = run_integration_template(source) if deltas is None: + # Direct dependency failure: surface a single concise + # inline marker so the user sees `[x] : integration + # failed` immediately (fixes "perceived hang" on HYBRID + # validation failures). The full diagnostic detail -- + # resolved path and `--verbose` hint -- is rendered once + # by `render_summary()` to avoid double-output. + if dep_key in direct_dep_keys: + if ctx.diagnostics: + ctx.diagnostics.error( + f"{dep_key}: integration failed", + package=dep_key, + detail=( + f"Resolved at {install_path}. " + f"Run with --verbose for details." + ), + ) + elif ctx.logger: + ctx.logger.error(f"{dep_key}: integration failed") + ctx.direct_dep_failed = True continue # Accumulate counter deltas from this package diff --git a/src/apm_cli/install/pipeline.py b/src/apm_cli/install/pipeline.py index e30b9ea9d..ff0061953 100644 --- a/src/apm_cli/install/pipeline.py +++ b/src/apm_cli/install/pipeline.py @@ -28,7 +28,7 @@ from ..models.results import InstallResult from ..utils.console import _rich_error from ..utils.diagnostics import DiagnosticCollector -from .errors import PolicyViolationError +from .errors import DirectDependencyError, PolicyViolationError if TYPE_CHECKING: from ..core.auth import AuthResolver @@ -320,6 +320,18 @@ def run_install_pipeline( _integrate_phase.run(ctx) + # Fail-loud: if any direct dependency failed validation or + # download, render the diagnostic summary and raise so the + # caller exits non-zero immediately. Transitive failures + # are allowed to proceed (log + continue). + if ctx.direct_dep_failed: + if ctx.diagnostics and ctx.diagnostics.has_diagnostics: + ctx.diagnostics.render_summary() + raise DirectDependencyError( + "One or more direct dependencies failed validation. " + "Run with --verbose for details." + ) + # Update .gitignore from apm_cli.commands._helpers import _update_gitignore_for_apm_modules @@ -365,5 +377,9 @@ def run_install_pipeline( # the typed exception lets the caller render the policy message # as-is. raise + except DirectDependencyError: + # #946: same pattern -- surface the message as-is instead of + # double-wrapping it through the generic RuntimeError below. + raise except Exception as e: raise RuntimeError(f"Failed to resolve APM dependencies: {e}") diff --git a/src/apm_cli/integration/skill_integrator.py b/src/apm_cli/integration/skill_integrator.py index 98c517fea..7ff84f8bb 100644 --- a/src/apm_cli/integration/skill_integrator.py +++ b/src/apm_cli/integration/skill_integrator.py @@ -327,7 +327,8 @@ def copy_skill_to_target( skill_dir.parent.mkdir(parents=True, exist_ok=True) if skill_dir.exists(): shutil.rmtree(skill_dir) - shutil.copytree(source_path, skill_dir) + from apm_cli.security.gate import ignore_symlinks + shutil.copytree(source_path, skill_dir, ignore=ignore_symlinks) deployed.append(skill_dir) return deployed @@ -565,7 +566,8 @@ def _promote_sub_skills(sub_skills_dir: Path, target_skills_root: Path, parent_n pass shutil.rmtree(target) target.mkdir(parents=True, exist_ok=True) - shutil.copytree(sub_skill_path, target, dirs_exist_ok=True) + from apm_cli.security.gate import ignore_symlinks + shutil.copytree(sub_skill_path, target, dirs_exist_ok=True, ignore=ignore_symlinks) promoted += 1 deployed.append(target) return promoted, deployed @@ -815,8 +817,19 @@ def _integrate_native_skill( shutil.rmtree(target_skill_dir) target_skill_dir.parent.mkdir(parents=True, exist_ok=True) + from apm_cli.security.gate import ignore_symlinks as _ignore_symlinks + _apm_filter = shutil.ignore_patterns('.apm') + + def _ignore_symlinks_and_apm(directory, contents): + # Compose two ignore filters: drop symlinks (security gate) + # AND drop nested `.apm/` so consumers of the bundle do not + # see the author's primitive layout leak into the deployed + # skill tree. + return list(set(_ignore_symlinks(directory, contents)) + | set(_apm_filter(directory, contents))) + shutil.copytree(package_path, target_skill_dir, - ignore=shutil.ignore_patterns('.apm')) + ignore=_ignore_symlinks_and_apm) all_target_paths.append(target_skill_dir) if is_primary: diff --git a/src/apm_cli/models/validation.py b/src/apm_cli/models/validation.py index 754eb42b0..3d0b72564 100644 --- a/src/apm_cli/models/validation.py +++ b/src/apm_cli/models/validation.py @@ -282,7 +282,10 @@ def validate_apm_package(package_path: Path) -> ValidationResult: if pkg_type == PackageType.INVALID: result.add_error( f"Not a valid APM package: no apm.yml, SKILL.md, hooks, or " - f"plugin structure found in {package_path.name}" + f"plugin structure found in {package_path.name}. " + "Ensure the package has SKILL.md (skill bundle), " + "apm.yml + .apm/ (APM package), or plugin.json (Claude plugin) " + "at its root." ) return result @@ -299,8 +302,15 @@ def validate_apm_package(package_path: Path) -> ValidationResult: if result.package_type == PackageType.MARKETPLACE_PLUGIN: return _validate_marketplace_plugin(package_path, plugin_json_path, result) - # Standard APM package validation (has apm.yml) + # Standard APM package or HYBRID validation (has apm.yml) apm_yml_path = package_path / APM_YML_FILENAME + + # HYBRID packages: if .apm/ exists, fall through to standard validation + # (back-compat for packages that ship both .apm/ primitives AND SKILL.md). + # Otherwise validate as a skill bundle with apm.yml metadata. + if result.package_type == PackageType.HYBRID: + return _validate_hybrid_package(package_path, apm_yml_path, result) + return _validate_apm_package_with_yml(package_path, apm_yml_path, result) @@ -375,6 +385,81 @@ def _validate_claude_skill(package_path: Path, skill_md_path: Path, result: Vali return result +def _validate_hybrid_package( + package_path: Path, apm_yml_path: Path, result: ValidationResult +) -> ValidationResult: + """Validate a HYBRID package (apm.yml + SKILL.md). + + Two sub-cases: + + 1. ``.apm/`` directory present -- fall through to the standard + ``_validate_apm_package_with_yml`` path for full back-compat. + 2. No ``.apm/`` -- treat as a *skill bundle* whose metadata comes from + ``apm.yml`` (authoritative for name/version/license/deps) and whose + runtime behavior is driven by ``SKILL.md``. This is the Genesis + layout: ``apm.yml`` + ``SKILL.md`` + optional sub-directories at + repo root, no ``.apm/``. + + Args: + package_path: Path to the package directory + apm_yml_path: Path to apm.yml + result: ValidationResult to populate + + Returns: + ValidationResult: Updated validation result + """ + # Back-compat: if .apm/ exists, the author intends independent primitives. + apm_dir = package_path / APM_DIR + if apm_dir.exists() and apm_dir.is_dir(): + return _validate_apm_package_with_yml(package_path, apm_yml_path, result) + + # --- Skill-bundle path (no .apm/) --- + from .apm_package import APMPackage + + # Parse apm.yml -- authoritative for APM-owned fields. + try: + package = APMPackage.from_apm_yml(apm_yml_path) + except (ValueError, FileNotFoundError) as e: + result.add_error(f"Invalid apm.yml: {e}") + return result + + # Require SKILL.md present and minimally readable. + skill_md_path = package_path / SKILL_MD_FILENAME + if not skill_md_path.exists(): + result.add_error(f"HYBRID package missing {SKILL_MD_FILENAME}") + return result + + try: + import frontmatter + + with open(skill_md_path, "r", encoding="utf-8") as f: + frontmatter.load(f) # Parse only to surface malformed frontmatter. + + # Metadata model for HYBRID packages: apm.yml.description and + # SKILL.md frontmatter description are INDEPENDENT fields with + # different consumers and MUST NOT be merged. + # + # * apm.yml.description -> human tagline rendered by `apm view`, + # `apm search`, `apm deps list`, marketplace/registry indexes. + # * SKILL.md description -> agent-runtime invocation matcher + # (per agentskills.io), consumed verbatim by Claude/Copilot/etc. + # APM never reads or mutates this field; the file is copied + # byte-for-byte into /skills// at integrate time. + # + # Authors who ship a HYBRID package are expected to populate both + # descriptions independently. The pack-time check in + # `apm_cli.bundle.packer` warns when apm.yml.description is missing + # so the human-facing surfaces (search/listings) do not degrade + # silently while the agent runtime keeps working. + + except Exception as e: + result.add_warning(f"Could not parse {SKILL_MD_FILENAME} frontmatter: {e}") + + result.package = package + # package_type already set to HYBRID by the caller + return result + + def _validate_marketplace_plugin(package_path: Path, plugin_json_path: Optional[Path], result: ValidationResult) -> ValidationResult: """Validate a Claude plugin and synthesize apm.yml. @@ -433,7 +518,12 @@ def _validate_apm_package_with_yml(package_path: Path, apm_yml_path: Path, resul # Check for .apm directory apm_dir = package_path / APM_DIR if not apm_dir.exists(): - result.add_error(f"Missing required directory: {APM_DIR}/") + result.add_error( + f"Missing required directory: {APM_DIR}/ -- " + "an APM package with apm.yml needs a .apm/ directory containing " + "primitives. Alternatively, add a SKILL.md to make this a skill " + "bundle or hybrid package." + ) return result if not apm_dir.is_dir(): diff --git a/tests/test_apm_package_models.py b/tests/test_apm_package_models.py index 17d3f297d..418547e60 100644 --- a/tests/test_apm_package_models.py +++ b/tests/test_apm_package_models.py @@ -1143,6 +1143,179 @@ def test_obra_superpowers_layout(self, tmp_path): assert pj_path.name == "plugin.json" +class TestHybridPackageValidation: + """Tests for HYBRID package validation (apm.yml + SKILL.md, no .apm/). + + Genesis-layout reproducer: apm.yml + SKILL.md + optional agents/ at + repo root, no .apm/ directory. validate_apm_package must return + package_type == HYBRID with no errors. + """ + + def test_hybrid_no_apm_dir_validates_as_skill_bundle(self, tmp_path): + """Core reproducer: HYBRID layout without .apm/ is valid.""" + (tmp_path / "apm.yml").write_text( + "name: genesis\n" + "version: 1.0.0\n" + "description: Genesis architect\n" + ) + (tmp_path / "SKILL.md").write_text( + "---\nname: genesis\ndescription: skill desc\n---\n# Genesis Skill\n" + ) + agents_dir = tmp_path / "agents" + agents_dir.mkdir() + (agents_dir / "genesis-architect.agent.md").write_text("# Agent") + + result = validate_apm_package(tmp_path) + assert result.is_valid, f"Expected valid but got errors: {result.errors}" + assert result.package_type == PackageType.HYBRID + assert result.package is not None + assert result.package.name == "genesis" + assert result.package.version == "1.0.0" + + def test_hybrid_with_apm_dir_falls_through_to_standard(self, tmp_path): + """HYBRID with .apm/ present uses standard APM package validation.""" + (tmp_path / "apm.yml").write_text( + "name: hybrid-classic\nversion: 2.0.0\n" + ) + (tmp_path / "SKILL.md").write_text("# Skill") + apm_dir = tmp_path / ".apm" + apm_dir.mkdir() + inst_dir = apm_dir / "instructions" + inst_dir.mkdir() + (inst_dir / "foo.instructions.md").write_text("# Foo") + + result = validate_apm_package(tmp_path) + assert result.is_valid, f"Expected valid but got errors: {result.errors}" + assert result.package_type == PackageType.HYBRID + + def test_hybrid_bad_apm_yml_reports_error(self, tmp_path): + """HYBRID with malformed apm.yml is invalid.""" + (tmp_path / "apm.yml").write_text("invalid: [yaml") + (tmp_path / "SKILL.md").write_text("# Skill") + + result = validate_apm_package(tmp_path) + assert not result.is_valid + assert any("Invalid apm.yml" in e for e in result.errors) + + def test_hybrid_skill_md_description_does_not_backfill_into_apm_yml(self, tmp_path): + """apm.yml.description and SKILL.md description are independent. + + SKILL.md is consumed by the agent runtime (invocation matcher per + agentskills.io); apm.yml.description is consumed by APM tooling + (`apm view`, search, listings). They serve different consumers + and APM never merges them. When apm.yml omits its description, + ``APMPackage.description`` stays ``None`` -- the SKILL.md value + does NOT silently leak into the human-facing tagline slot. + """ + (tmp_path / "apm.yml").write_text( + "name: genesis\nversion: 1.0.0\n" + ) + (tmp_path / "SKILL.md").write_text( + "---\ndescription: from-skill-md\n---\n# Skill\n" + ) + + result = validate_apm_package(tmp_path) + assert result.is_valid + assert result.package.description is None + + def test_hybrid_apm_yml_description_wins_over_skill_md(self, tmp_path): + """apm.yml.description is the only source for APMPackage.description. + + When apm.yml provides a description, that value is used verbatim + regardless of SKILL.md frontmatter -- there is no merge. + """ + (tmp_path / "apm.yml").write_text( + "name: genesis\nversion: 1.0.0\ndescription: from-apm-yml\n" + ) + (tmp_path / "SKILL.md").write_text( + "---\ndescription: from-skill-md\n---\n# Skill\n" + ) + + result = validate_apm_package(tmp_path) + assert result.is_valid + assert result.package.description == "from-apm-yml" + + def test_hybrid_both_descriptions_independent(self, tmp_path): + """SKILL.md content is preserved on disk untouched after validation. + + APM must never mutate the SKILL.md file; the agent runtime reads + it byte-for-byte from `/skills//SKILL.md` after + integration. This test asserts (a) APMPackage.description comes + only from apm.yml and (b) SKILL.md is untouched on disk. + """ + skill_md_content = ( + "---\n" + "name: genesis\n" + "description: This skill should be invoked when the user asks " + "about Genesis architecture decisions.\n" + "allowed-tools: [bash, view]\n" + "---\n" + "# Genesis Skill\n" + ) + (tmp_path / "apm.yml").write_text( + "name: genesis\nversion: 1.0.0\ndescription: short tagline\n" + ) + (tmp_path / "SKILL.md").write_text(skill_md_content) + + result = validate_apm_package(tmp_path) + assert result.is_valid + assert result.package.description == "short tagline" + # SKILL.md must be untouched -- the agent runtime reads it verbatim. + assert (tmp_path / "SKILL.md").read_text() == skill_md_content + + +class TestClaudeSkillPackageValidation: + """Tests for CLAUDE_SKILL packages (SKILL.md only, no apm.yml). + + Verifies the ``SKILL.md + agents/ + assets/`` layout (no apm.yml) + classifies as CLAUDE_SKILL and is NOT misclassified as + MARKETPLACE_PLUGIN even though ``agents/`` is in ``_PLUGIN_DIRS``. + """ + + def test_claude_skill_with_agents_and_assets_validates(self, tmp_path): + """CLAUDE_SKILL with agents/ and assets/ sub-dirs passes validation.""" + (tmp_path / "SKILL.md").write_text( + "---\nname: my-skill\ndescription: A skill with agents\n---\n" + "# My Skill\n" + ) + agents_dir = tmp_path / "agents" + agents_dir.mkdir() + (agents_dir / "foo.agent.md").write_text("# Foo Agent") + assets_dir = tmp_path / "assets" + assets_dir.mkdir() + (assets_dir / "logo.png").write_bytes(b"\x89PNG") + + result = validate_apm_package(tmp_path) + assert result.is_valid, f"Expected valid but got errors: {result.errors}" + assert result.package_type == PackageType.CLAUDE_SKILL + assert result.package is not None + assert result.package.name == "my-skill" + + def test_claude_skill_with_agents_dir_not_misclassified_as_plugin(self, tmp_path): + """SKILL.md presence beats agents/ directory in the detection cascade. + + ``agents/`` is in ``_PLUGIN_DIRS``, so without SKILL.md it would + classify as MARKETPLACE_PLUGIN. With SKILL.md present the cascade + must short-circuit to CLAUDE_SKILL (step 3 precedes step 4). + """ + (tmp_path / "SKILL.md").write_text( + "---\nname: agents-skill\n---\n# Has Agents\n" + ) + (tmp_path / "agents").mkdir() + (tmp_path / "agents" / "bar.agent.md").write_text("# Bar Agent") + + # Detection level + pkg_type, pj = detect_package_type(tmp_path) + assert pkg_type == PackageType.CLAUDE_SKILL + assert pj is None + + # Full validation level + result = validate_apm_package(tmp_path) + assert result.is_valid, f"Expected valid but got errors: {result.errors}" + assert result.package_type == PackageType.CLAUDE_SKILL + assert result.package_type != PackageType.MARKETPLACE_PLUGIN + + class TestGatherDetectionEvidence: """Tests for the evidence-gathering helper that powers observability.""" diff --git a/tests/unit/install/test_direct_dep_failure.py b/tests/unit/install/test_direct_dep_failure.py new file mode 100644 index 000000000..59136352c --- /dev/null +++ b/tests/unit/install/test_direct_dep_failure.py @@ -0,0 +1,136 @@ +"""Tests for the fail-loud direct-dependency integration failure path. + +When ``run_integration_template`` returns ``None`` for a direct dependency, +the integrate phase must set ``ctx.direct_dep_failed``, push an error to +the diagnostic collector, and the pipeline must raise +``DirectDependencyError`` so the CLI exits non-zero (#946). +""" + +from __future__ import annotations + +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest + +from apm_cli.install.context import InstallContext +from apm_cli.install.errors import DirectDependencyError +from apm_cli.utils.diagnostics import DiagnosticCollector + + +def _make_ctx(tmp_path: Path, dep_keys: list[str]) -> InstallContext: + """Build a minimal InstallContext with fake deps whose keys match *dep_keys*.""" + project_root = tmp_path / "project" + project_root.mkdir() + apm_dir = project_root / ".apm" + apm_dir.mkdir() + modules = project_root / "apm_modules" + modules.mkdir() + + deps = [] + for key in dep_keys: + dep = MagicMock() + dep.get_unique_key.return_value = key + dep.alias = None + dep.is_local = False + dep.local_path = None + dep.get_install_path.return_value = modules / key + deps.append(dep) + + ctx = InstallContext( + project_root=project_root, + apm_dir=apm_dir, + ) + ctx.apm_modules_dir = modules + ctx.deps_to_install = list(deps) + ctx.all_apm_deps = list(deps) + ctx.diagnostics = DiagnosticCollector() + ctx.logger = MagicMock() + ctx.targets = [] + ctx.integrators = {} + ctx.installed_packages = [] + return ctx + + +class TestDirectDepFailLoud: + """Validate the fail-loud path when a direct dep fails integration.""" + + @patch("apm_cli.install.phases.integrate.run_integration_template", return_value=None) + @patch("apm_cli.install.phases.integrate.make_dependency_source") + def test_integrate_sets_direct_dep_failed_on_none_deltas( + self, _mock_source, _mock_template, tmp_path + ): + """When run_integration_template returns None for a direct dep, + ctx.direct_dep_failed must be True.""" + from apm_cli.install.phases.integrate import run + + ctx = _make_ctx(tmp_path, ["owner/pkg"]) + run(ctx) + + assert ctx.direct_dep_failed is True + + @patch("apm_cli.install.phases.integrate.run_integration_template", return_value=None) + @patch("apm_cli.install.phases.integrate.make_dependency_source") + def test_integrate_pushes_error_to_diagnostics( + self, _mock_source, _mock_template, tmp_path + ): + """The diagnostic collector must record the failure.""" + from apm_cli.install.phases.integrate import run + + ctx = _make_ctx(tmp_path, ["owner/pkg"]) + run(ctx) + + assert ctx.diagnostics.has_diagnostics + assert ctx.diagnostics.error_count >= 1 + + def test_pipeline_raises_direct_dependency_error(self, tmp_path): + """run_install_pipeline must raise DirectDependencyError (not + generic RuntimeError) when ctx.direct_dep_failed is set by the + integrate phase.""" + from apm_cli.install.pipeline import run_install_pipeline + + pkg = MagicMock() + pkg.dependencies = {"apm": []} + + def _fake_integrate_run(ctx): + """Simulate the integrate phase setting the failure flag.""" + ctx.direct_dep_failed = True + + with ( + patch("apm_cli.install.phases.integrate.run", side_effect=_fake_integrate_run), + patch("apm_cli.install.phases.resolve.run"), + patch("apm_cli.install.phases.targets.run"), + patch("apm_cli.install.phases.download.run"), + ): + with pytest.raises(DirectDependencyError): + run_install_pipeline( + apm_package=pkg, + verbose=False, + logger=MagicMock(), + ) + + @patch("apm_cli.install.phases.integrate.run_integration_template") + @patch("apm_cli.install.phases.integrate.make_dependency_source") + def test_transitive_failure_does_not_set_flag( + self, _mock_source, mock_template, tmp_path + ): + """When a transitive (non-direct) dep returns None, direct_dep_failed + must stay False -- only direct deps trigger the fail-loud path.""" + from apm_cli.install.phases.integrate import run + + mock_template.return_value = None + + # Build ctx with a transitive dep whose key differs from all_apm_deps + ctx = _make_ctx(tmp_path, dep_keys=[]) + transitive = MagicMock() + transitive.get_unique_key.return_value = "other/transitive" + transitive.alias = None + transitive.is_local = False + transitive.local_path = None + transitive.get_install_path.return_value = tmp_path / "apm_modules" / "other" / "transitive" + ctx.deps_to_install = [transitive] + # all_apm_deps is empty => transitive key not in direct_dep_keys + + run(ctx) + + assert ctx.direct_dep_failed is False diff --git a/tests/unit/test_packer.py b/tests/unit/test_packer.py index 920fb2b02..946cbf156 100644 --- a/tests/unit/test_packer.py +++ b/tests/unit/test_packer.py @@ -937,3 +937,92 @@ def test_enrich_lockfile_strips_local_fields(self): # Original lockfile object is not mutated assert lock.local_deployed_files == [".github/agents/own.md"] assert lock.local_deployed_file_hashes == {".github/agents/own.md": "h"} + + +class TestPackHybridDescriptionWarning: + """pack_bundle warns when a HYBRID package is missing apm.yml.description. + + HYBRID layout = apm.yml + SKILL.md at project root (no `.apm/`). + apm.yml.description and SKILL.md description are independent fields + with different consumers (CLI/search vs. agent invocation matcher). + A package can pack and ship without apm.yml.description, but its + `apm view` / `apm search` / marketplace listing will degrade to + "(no description)" while Claude/Copilot still invoke the skill + correctly. The pack-time warning catches this for the AUTHOR -- the + only actor who can fix it -- without forcing every CONSUMER to see + noise on `apm install`. + """ + + def _build_hybrid(self, tmp_path: Path, *, apm_desc: str | None, skill_desc: str | None) -> Path: + project = tmp_path / "project" + project.mkdir() + apm_yml: dict = {"name": "genesis", "version": "1.0.0"} + if apm_desc is not None: + apm_yml["description"] = apm_desc + (project / "apm.yml").write_text(yaml.dump(apm_yml), encoding="utf-8") + skill_body = "---\n" + if skill_desc is not None: + skill_body += f"description: {skill_desc}\n" + skill_body += "---\n# Skill\n" + (project / "SKILL.md").write_text(skill_body, encoding="utf-8") + + lockfile = LockFile() + dep = LockedDependency(repo_url="owner/repo", deployed_files=[]) + lockfile.add_dependency(dep) + lockfile.write(project / "apm.lock.yaml") + return project + + def test_warning_fires_when_apm_yml_missing_description(self, tmp_path): + """SKILL.md has description, apm.yml does not -- warn the author.""" + project = self._build_hybrid( + tmp_path, apm_desc=None, skill_desc="Genesis architecture skill" + ) + out = tmp_path / "build" + + recorded: list[str] = [] + + class _Logger: + def warning(self, msg): + recorded.append(msg) + + pack_bundle(project, out, dry_run=True, logger=_Logger()) + + assert any("apm.yml is missing 'description'" in m for m in recorded), ( + f"Expected pack-time HYBRID warning, got: {recorded}" + ) + + def test_no_warning_when_apm_yml_has_description(self, tmp_path): + """apm.yml.description present -- silent regardless of SKILL.md.""" + project = self._build_hybrid( + tmp_path, apm_desc="short tagline", skill_desc="long invocation matcher" + ) + out = tmp_path / "build" + + recorded: list[str] = [] + + class _Logger: + def warning(self, msg): + recorded.append(msg) + + pack_bundle(project, out, dry_run=True, logger=_Logger()) + + assert not any("missing 'description'" in m for m in recorded), ( + f"Did not expect HYBRID warning, got: {recorded}" + ) + + def test_no_warning_when_skill_md_also_missing_description(self, tmp_path): + """If neither has description, no warning -- nothing to nudge about.""" + project = self._build_hybrid(tmp_path, apm_desc=None, skill_desc=None) + out = tmp_path / "build" + + recorded: list[str] = [] + + class _Logger: + def warning(self, msg): + recorded.append(msg) + + pack_bundle(project, out, dry_run=True, logger=_Logger()) + + assert not any("missing 'description'" in m for m in recorded), ( + f"Did not expect HYBRID warning when SKILL.md also lacks description, got: {recorded}" + ) diff --git a/tests/unit/test_symlink_containment.py b/tests/unit/test_symlink_containment.py index 12392ac18..752812268 100644 --- a/tests/unit/test_symlink_containment.py +++ b/tests/unit/test_symlink_containment.py @@ -184,5 +184,91 @@ def test_symlinked_hook_json_outside_package_rejected(self): self.assertNotIn("evil.json", names) +class TestSkillIntegratorCopytreeSymlinkContainment(unittest.TestCase): + """skill_integrator copytree paths drop symlinks via security gate. + + Three copytree call sites in + `apm_cli.integration.skill_integrator` deploy a skill bundle into + target directories. Any of them must drop symlinks contained in + the source tree -- otherwise a malicious package could ship a + symlink pointing at `/etc/passwd` (or any path outside the + package) and have it materialised inside the target's runtime + directory after `apm install`. + + These tests do not exercise the higher-level integrator pipeline; + they validate the contract by importing + `apm_cli.security.gate.ignore_symlinks` and confirming each call + site invokes ``shutil.copytree`` with that callback (or a + composition that includes it). + """ + + def setUp(self): + self.tmpdir = tempfile.mkdtemp() + self.src = Path(self.tmpdir) / "src" + self.src.mkdir() + self.outside = Path(self.tmpdir) / "outside" + self.outside.mkdir() + self.secret = self.outside / "secret.txt" + self.secret.write_text("sensitive", encoding="utf-8") + self.dest = Path(self.tmpdir) / "dest" + + def tearDown(self): + shutil.rmtree(self.tmpdir, ignore_errors=True) + + def _build_source_with_symlink(self): + """Create a source dir with a real file plus a symlink to outside.""" + (self.src / "real.md").write_text("real", encoding="utf-8") + nested = self.src / "agents" + nested.mkdir() + (nested / "real.agent.md").write_text("real", encoding="utf-8") + link = self.src / "evil.txt" + _try_symlink(link, self.secret) + nested_link = nested / "evil-nested.txt" + _try_symlink(nested_link, self.secret) + + def test_ignore_symlinks_callback_excludes_top_level_and_nested(self): + """The shared ignore_symlinks callback drops symlinks at every depth.""" + from apm_cli.security.gate import ignore_symlinks + + self._build_source_with_symlink() + shutil.copytree(self.src, self.dest, ignore=ignore_symlinks) + + copied = sorted(p.relative_to(self.dest).as_posix() for p in self.dest.rglob("*")) + self.assertIn("real.md", copied) + self.assertIn("agents", copied) + self.assertIn("agents/real.agent.md", copied) + self.assertNotIn("evil.txt", copied) + self.assertNotIn("agents/evil-nested.txt", copied) + + def test_skill_integrator_native_skill_copytree_uses_ignore_symlinks(self): + """integrate_native_skills passes ignore_symlinks to shutil.copytree. + + Source-level guard: if a future refactor drops the callback, + this test fails before any malicious package can exploit it. + """ + import inspect + from apm_cli.integration import skill_integrator + + source = inspect.getsource(skill_integrator) + # All three copytree calls in skill_integrator.py must reference + # ignore_symlinks (directly or via a composing helper). + copytree_count = source.count("shutil.copytree(") + ignore_symlinks_refs = source.count("ignore_symlinks") + self.assertGreaterEqual( + copytree_count, + 3, + f"Expected >=3 copytree calls in skill_integrator, found {copytree_count}", + ) + # Each copytree must be matched by at least one ignore_symlinks + # reference (the helper at line 818 composes one ignore_symlinks + # import + one usage inside a closure -- still >=copytree_count). + self.assertGreaterEqual( + ignore_symlinks_refs, + copytree_count, + f"Expected >={copytree_count} ignore_symlinks references " + f"(one per copytree); found {ignore_symlinks_refs}", + ) + + if __name__ == "__main__": unittest.main()