diff --git a/CHANGELOG.md b/CHANGELOG.md index bddeb38be..6386777b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- `apm install --skill X` is now properly additive: a later + `apm install --skill Y` adds `Y` on top of the existing pin instead + of silently removing the previously deployed `X` from disk. Deployment now + unions the persisted `apm.yml` `skills:` pin with the current `--skill` + values (matching the persistence behavior shipped in #1786), the lockfile + records the same union, and `--skill '*'` resets the pin back to the full + bundle. Drop a single skill by editing the `skills:` list in `apm.yml` and + re-running `apm install`. (#1955) + ## [0.23.0] - 2026-06-28 ### Added diff --git a/docs/src/content/docs/reference/cli/install.md b/docs/src/content/docs/reference/cli/install.md index 25d6651e9..0a27bd8d8 100644 --- a/docs/src/content/docs/reference/cli/install.md +++ b/docs/src/content/docs/reference/cli/install.md @@ -80,7 +80,7 @@ Transport env vars: `APM_GIT_PROTOCOL` (`ssh` or `https`) sets the default initi | Flag | Default | Description | |---|---|---| -| `--skill NAME` | all | Install only named skill(s) from a skill collection (`SKILL_BUNDLE` or plugin manifest). Repeatable. For plugin manifests, `NAME` may be the skill name or manifest path, such as `skills/productivity/grill-me`. The selection is persisted to `apm.yml` and `apm.lock.yaml`. Use `--skill '*'` to reset and install all. | +| `--skill NAME` | all | Install only named skill(s) from a skill collection (`SKILL_BUNDLE` or plugin manifest). Repeatable. For plugin manifests, `NAME` may be the skill name or manifest path, such as `skills/productivity/grill-me`. The selection is persisted to `apm.yml` and `apm.lock.yaml`. `--skill` is additive across separate installs: a later `apm install --skill X` adds `X` to the existing pin (union) rather than replacing it -- previously deployed skills are never silently removed. Use `--skill '*'` to reset to the full bundle; to drop a single skill, edit the `skills:` list in `apm.yml` and re-run `apm install`. | | `--as ALIAS` | bundle id | Override the log/display label for a local-bundle install. Only valid with a single local-bundle `PACKAGE_REF`. | ### MCP server entry (use only with `--mcp`) @@ -186,8 +186,9 @@ apm install ./my-bundle --target opencode ### Install only a subset of skills from a bundle ```bash -apm install owner/skill-bundle --skill review --skill refactor -apm install owner/skill-bundle --skill '*' # reset to all skills +apm install owner/skill-bundle --skill review +apm install owner/skill-bundle --skill refactor # adds refactor; review is kept (union) +apm install owner/skill-bundle --skill '*' # reset to all skills ``` ## Exit codes diff --git a/docs/src/content/docs/reference/package-types.md b/docs/src/content/docs/reference/package-types.md index 3c23f45ff..11612a671 100644 --- a/docs/src/content/docs/reference/package-types.md +++ b/docs/src/content/docs/reference/package-types.md @@ -128,7 +128,10 @@ installing N separate CLAUDE_SKILL packages. from the bundle (repeatable). The selection is **persisted** in `apm.yml` (as a `skills:` field) and `apm.lock.yaml` (as `skill_subset`), so subsequent bare `apm install` commands are deterministic. -Use `--skill '*'` to reset and install all skills. +Use `--skill '*'` to reset and install all skills. `--skill` is additive +across separate installs (a later `--skill X` unions onto the existing pin +and never removes already-deployed skills) -- see +[apm install](../cli/install/). ```bash # Install only two skills (persisted to apm.yml): diff --git a/packages/apm-guide/.apm/skills/apm-usage/commands.md b/packages/apm-guide/.apm/skills/apm-usage/commands.md index 7cab4dfa5..0a52c8fd6 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 APM and MCP dependencies (supports APM packages, Claude skills (SKILL.md), and plugin collections (plugin.json)) | `--update` (deprecated; prefer `apm update`) refresh refs, `--refresh` re-fetch all deps from upstream and re-resolve all ref pins, `--force` overwrite (does NOT refresh refs; use `apm update` for that), `--frozen` CI-safe install that fails fast when `apm.lock.yaml` is missing or out of sync with `apm.yml` (mutually exclusive with `--update`; structural presence check only -- use `apm audit` for SHA integrity), `--dry-run`, `--verbose`, `--only [apm\|mcp]`, `--target` (comma-separated, e.g. `--target claude,cursor`; highest-priority entry in the resolution chain `--target` > apm.yml `targets:` > auto-detect; on auto-bootstrap when no `apm.yml` exists, recognized manifest target(s) are persisted to the new manifest's `targets:` so a later bare `apm update` reuses them; `--target all` deprecated, see `apm compile --all`; use `kiro` for Kiro IDE; use `copilot-cowork` with `--global` after `apm experimental enable copilot-cowork`; use `hermes` after `apm experimental enable hermes` to deploy skills + `AGENTS.md` and, at `--global`, MCP servers to `~/.hermes/config.yaml`), `--dev`, `-g` global (MCP deploys only to user-scope runtimes: Copilot CLI, Claude Code, Codex CLI, Gemini CLI, Antigravity CLI, Kiro, Windsurf, JetBrains Copilot, and Hermes when enabled), `--trust-transitive-mcp`, `--parallel-downloads N`, `--allow-insecure`, `--allow-insecure-host HOSTNAME`, `--skill NAME` install named skill(s) from a skill collection (SKILL_BUNDLE or plugin manifest; repeatable; plugin manifests accept a leaf name or manifest path; persisted in apm.yml; `'*'` resets to all), `--legacy-skill-paths` restore per-client skill dirs, `--mcp NAME` add MCP entry (NAME goes through the same `--target` > `targets:` > auto-detect resolver as APM packages, so a project whitelisting `targets: [copilot]` will not write `.cursor/mcp.json` even if `.cursor/` exists; `apm install -g --mcp NAME` writes user-scope and bypasses the project-scope gate by design), `--transport`, `--url`, `--env KEY=VAL`, `--header KEY=VAL`, `--mcp-version`, `--registry URL` custom MCP registry, `--root DIR` redirect writes (`apm_modules/`, lockfile, `.gitignore`, integrated harness files) under DIR while `apm.yml`/`.apm/`/local deps resolve from `$PWD` (mirrors `pip install --target`; created if missing; not valid with `-g`/`--global`, which exits 2) | +| `apm install [PKGS...]` | Install APM and MCP dependencies (supports APM packages, Claude skills (SKILL.md), and plugin collections (plugin.json)) | `--update` (deprecated; prefer `apm update`) refresh refs, `--refresh` re-fetch all deps from upstream and re-resolve all ref pins, `--force` overwrite (does NOT refresh refs; use `apm update` for that), `--frozen` CI-safe install that fails fast when `apm.lock.yaml` is missing or out of sync with `apm.yml` (mutually exclusive with `--update`; structural presence check only -- use `apm audit` for SHA integrity), `--dry-run`, `--verbose`, `--only [apm\|mcp]`, `--target` (comma-separated, e.g. `--target claude,cursor`; highest-priority entry in the resolution chain `--target` > apm.yml `targets:` > auto-detect; on auto-bootstrap when no `apm.yml` exists, recognized manifest target(s) are persisted to the new manifest's `targets:` so a later bare `apm update` reuses them; `--target all` deprecated, see `apm compile --all`; use `kiro` for Kiro IDE; use `copilot-cowork` with `--global` after `apm experimental enable copilot-cowork`; use `hermes` after `apm experimental enable hermes` to deploy skills + `AGENTS.md` and, at `--global`, MCP servers to `~/.hermes/config.yaml`), `--dev`, `-g` global (MCP deploys only to user-scope runtimes: Copilot CLI, Claude Code, Codex CLI, Gemini CLI, Antigravity CLI, Kiro, Windsurf, JetBrains Copilot, and Hermes when enabled), `--trust-transitive-mcp`, `--parallel-downloads N`, `--allow-insecure`, `--allow-insecure-host HOSTNAME`, `--skill NAME` install named skill(s) from a skill collection (SKILL_BUNDLE or plugin manifest; repeatable; plugin manifests accept a leaf name or manifest path; persisted in apm.yml; additive across separate installs -- a later `--skill X` adds to the existing pin (union) rather than replacing it, so previously deployed skills are never silently removed; `'*'` resets to the full bundle; drop a single skill by editing the `skills:` list in apm.yml then re-running install), `--legacy-skill-paths` restore per-client skill dirs, `--mcp NAME` add MCP entry (NAME goes through the same `--target` > `targets:` > auto-detect resolver as APM packages, so a project whitelisting `targets: [copilot]` will not write `.cursor/mcp.json` even if `.cursor/` exists; `apm install -g --mcp NAME` writes user-scope and bypasses the project-scope gate by design), `--transport`, `--url`, `--env KEY=VAL`, `--header KEY=VAL`, `--mcp-version`, `--registry URL` custom MCP registry, `--root DIR` redirect writes (`apm_modules/`, lockfile, `.gitignore`, integrated harness files) under DIR while `apm.yml`/`.apm/`/local deps resolve from `$PWD` (mirrors `pip install --target`; created if missing; not valid with `-g`/`--global`, which exits 2) | | `apm targets` | Show resolved deployment targets for the current project (Click group; reads filesystem signals; works with or without `apm.yml`) | `--all` also include the `agent-skills` meta-target (only meaningful with `--json`), `--json` machine-readable output. No provenance line is printed (the table is the provenance). | | `apm uninstall PKGS...` | Remove packages (accepts `owner/repo` or `name@marketplace`) | `--dry-run`, `-g` global | | `apm prune` | Remove orphaned packages | `--dry-run` | diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index e80848298..d22a09751 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -47,8 +47,9 @@ from apm_cli.install.mcp.writer import _add_mcp_to_apm_yml # noqa: F401 from apm_cli.install.package_resolution import ( GIT_PARENT_USER_SCOPE_ERROR, + apply_cli_skill_pin, + cli_skill_subset, dependency_reference_to_yaml_entry, - normalize_and_merge_skill_subset, persist_dependency_list_if_changed, resolve_parsed_dependency_reference, update_existing_dependency_entry_if_needed, @@ -329,6 +330,7 @@ def _resolve_package_references( scope=None, allow_insecure=False, skill_subset=None, + skill_subset_from_cli=False, default_registry=None, ): """Validate, canonicalize, and resolve package references. @@ -453,16 +455,15 @@ def warning_handler(msg): ) canonical = dep_ref.to_canonical() identity = dep_ref.get_identity() - # Attach --skill filter so to_apm_yml_entry() emits the dict form. - # Merges with existing skills: list so repeated --skill - # invocations are additive (issue #1771). - if skill_subset: - dep_ref.skill_subset = normalize_and_merge_skill_subset( - skill_subset, - current_deps, - identity, - dependency_reference_cls=DependencyReference, - ) + apply_cli_skill_pin( + dep_ref, + skill_subset, + skill_subset_from_cli, + current_deps, + _apm_yml_entries, + dependency_reference_cls=DependencyReference, + logger=logger, + ) if marketplace_dep_ref is not None or direct_virtual_resolved: _apm_yml_entries[canonical] = dependency_reference_to_yaml_entry(dep_ref) except ValueError as e: @@ -633,6 +634,7 @@ def _validate_and_add_packages_to_apm_yml( scope=None, allow_insecure=False, skill_subset=None, + skill_subset_from_cli=False, ): """Validate packages exist and can be accessed, then add to apm.yml dependencies section. @@ -701,6 +703,7 @@ def _validate_and_add_packages_to_apm_yml( scope=scope, allow_insecure=allow_insecure, skill_subset=skill_subset, + skill_subset_from_cli=skill_subset_from_cli, default_registry=_default_registry_for_cli, ) @@ -1048,7 +1051,7 @@ def _handle_mcp_install( "skill_names", multiple=True, metavar="NAME", - help="Install only named skill(s) from a SKILL_BUNDLE. Repeatable. Persisted in apm.yml and apm.lock so bare 'apm install' is deterministic. Use --skill '*' to reset to all skills.", + help="Install only named skill(s) from a SKILL_BUNDLE. Repeatable. Persisted in apm.yml and apm.lock so bare 'apm install' is deterministic. Additive across installs: a later --skill X adds X to the existing pin (union) rather than replacing it. Use --skill '*' (quote the asterisk in your shell) to reset to all skills; to drop a single skill, edit the skills: list in apm.yml then re-run apm install.", ) @click.option( "--no-policy", @@ -1384,12 +1387,9 @@ def install( # noqa: PLR0913 ) # Normalize --skill: '*' means all (same as absent). Reject with --mcp. - _skill_subset = None - if skill_names: - if mcp_name is not None: - raise click.UsageError("--skill cannot be combined with --mcp.") - if not any(s == "*" for s in skill_names): - _skill_subset = builtins.tuple(skill_names) + if skill_names and mcp_name is not None: + raise click.UsageError("--skill cannot be combined with --mcp.") + _skill_subset = cli_skill_subset(skill_names) if mcp_name is not None: _handle_mcp_install( @@ -1517,6 +1517,7 @@ def install( # noqa: PLR0913 scope=scope, allow_insecure=allow_insecure, skill_subset=_skill_subset, + skill_subset_from_cli=bool(skill_names), ) # Short-circuit: all packages failed validation -- nothing to install if outcome.all_failed: diff --git a/src/apm_cli/install/package_resolution.py b/src/apm_cli/install/package_resolution.py index 52b4ad5d8..4a75b9e3f 100644 --- a/src/apm_cli/install/package_resolution.py +++ b/src/apm_cli/install/package_resolution.py @@ -180,6 +180,87 @@ def normalize_and_merge_skill_subset( return sorted(seen) +def effective_deploy_skill_subset( + *, + skill_subset_from_cli: bool, + cli_subset: builtins.tuple[str, ...] | builtins.list[str] | None, + persisted_subset: builtins.tuple[str, ...] | builtins.list[str] | None, +) -> builtins.tuple[str, ...] | None: + """Resolve the skill subset to *deploy* for a SKILL_BUNDLE. + + ``--skill`` is additive (issue #1786): a targeted ``--skill B`` install must + land on top of skills already pinned for the bundle instead of erasing them. + The deployment therefore uses the union of the persisted manifest subset and + the current CLI ``--skill`` values -- not the raw CLI subset alone. + + ``--skill '*'`` (signalled by ``skill_subset_from_cli`` True with an empty + ``cli_subset``) resets the bundle to its full set, so this returns ``None`` + meaning "deploy all skills". + + Returns a sorted, deduplicated tuple, or ``None`` for "deploy all". + """ + if skill_subset_from_cli and not cli_subset: + return None # --skill '*' -> full bundle + merged: builtins.set[str] = builtins.set() + if persisted_subset: + merged.update(persisted_subset) + if cli_subset: + merged.update(cli_subset) + return builtins.tuple(sorted(merged)) or None + + +def cli_skill_subset( + skill_names: builtins.tuple[str, ...], +) -> builtins.tuple[str, ...] | None: + """Resolve raw CLI ``--skill`` names to a subset, or None for absent / ``'*'``. + + ``--skill '*'`` means "all skills" (same as absent); the resolver still + learns the flag was present via a separate CLI-origin flag, so this + collapses both the absent and ``'*'`` cases to None. + """ + if skill_names and not any(s == "*" for s in skill_names): + return builtins.tuple(skill_names) + return None + + +def apply_cli_skill_pin( + dep_ref: Any, + cli_subset: builtins.tuple[str, ...] | None, + skill_subset_from_cli: bool, + current_deps: builtins.list, + apm_yml_entries: dict, + *, + dependency_reference_cls: Any, + logger: Any | None = None, +) -> None: + """Attach, merge, or reset a CLI ``--skill`` pin on ``dep_ref`` in place. + + With an explicit ``cli_subset``, merge it additively with any persisted + ``skills:`` so repeated ``--skill`` invocations union rather than replace + (issue #1771). With ``--skill '*'`` (``skill_subset_from_cli`` True and an + empty ``cli_subset``), reset the pin back to the full bundle and record the + refreshed plain-string ``apm.yml`` entry under the reference's canonical key + so manifest and on-disk state agree on the whole bundle (issue #1786 reset). + """ + identity = dep_ref.get_identity() + if cli_subset: + dep_ref.skill_subset = normalize_and_merge_skill_subset( + cli_subset, + current_deps, + identity, + dependency_reference_cls=dependency_reference_cls, + ) + return + if skill_subset_from_cli: + dep_ref.skill_subset = None + apm_yml_entries[dep_ref.to_canonical()] = dep_ref.to_apm_yml_entry() + if logger: + logger.verbose_detail( + f" [i] {identity}: skill pin reset to full bundle " + "(--skill '*'); a later bare 'apm install' deploys all skills" + ) + + def manifest_has_different_entry_for_identity( current_deps: builtins.list, identity: str, diff --git a/src/apm_cli/install/phases/lockfile.py b/src/apm_cli/install/phases/lockfile.py index b80711f87..e5c47b1f8 100644 --- a/src/apm_cli/install/phases/lockfile.py +++ b/src/apm_cli/install/phases/lockfile.py @@ -19,6 +19,7 @@ from pathlib import Path from typing import TYPE_CHECKING +from apm_cli.install.package_resolution import effective_deploy_skill_subset from apm_cli.utils.content_hash import compute_file_hash if TYPE_CHECKING: @@ -228,18 +229,31 @@ def _attach_exec_status(self, lockfile: LockFile) -> None: lockfile.dependencies[dep_key].exec_status = status def _attach_skill_subset_override(self, lockfile: LockFile) -> None: - """Apply CLI --skill override to lockfile skill_bundle entries. + """Union the CLI ``--skill`` values into lockfile skill_bundle entries. - When the user runs `apm install bundle --skill foo`, the CLI - skill_subset takes precedence over the per-entry skill_subset - from the manifest for this invocation's lockfile. + ``--skill`` is additive (issue #1786): a targeted ``apm install bundle + --skill foo`` must record the UNION of the previously pinned skills and + ``foo`` -- not replace the persisted subset with the raw CLI value. The + per-entry manifest subset already flows into ``locked_dep.skill_subset``; + here we fold in the current CLI values so the lockfile matches what was + deployed. + + ``--skill '*'`` (empty ``ctx.skill_subset``) is a no-op here -- the + manifest reset already flowed the full-bundle (empty) subset through. """ if not self.ctx.skill_subset: - return # No CLI override; dep_ref.skill_subset already flows through - effective = sorted(set(self.ctx.skill_subset)) + return # bare install or --skill '*'; manifest value already flows through for dep_key, locked_dep in lockfile.dependencies.items(): # noqa: B007 if locked_dep.package_type == "skill_bundle": - locked_dep.skill_subset = effective + merged = effective_deploy_skill_subset( + skill_subset_from_cli=self.ctx.skill_subset_from_cli, + cli_subset=self.ctx.skill_subset, + persisted_subset=locked_dep.skill_subset, + ) + # merged is never None here: the guard above returns for + # --skill '*' (empty ctx.skill_subset), so a real named + # subset always survives the union. + locked_dep.skill_subset = list(merged) if merged else [] def _attach_content_hashes(self, lockfile: LockFile) -> None: for dep_key, locked_dep in lockfile.dependencies.items(): diff --git a/src/apm_cli/install/template.py b/src/apm_cli/install/template.py index 50a3b7793..e6641b77a 100644 --- a/src/apm_cli/install/template.py +++ b/src/apm_cli/install/template.py @@ -14,6 +14,7 @@ from __future__ import annotations from apm_cli.install.helpers.security_scan import _pre_deploy_security_scan +from apm_cli.install.package_resolution import effective_deploy_skill_subset from apm_cli.install.services import IntegratorBundle, integrate_package_primitives from apm_cli.install.sources import DependencySource, Materialization @@ -128,6 +129,26 @@ def _integrate_materialization( ctx.package_deployed_files[dep_key] = [] return deltas + # Per-package effective subset: ``--skill`` is additive (issue + # #1786), so deploy the UNION of the persisted apm.yml ``skills:`` + # and the current CLI ``--skill`` values -- a targeted ``--skill`` + # install lands on top of previously pinned skills instead of + # erasing them. ``--skill '*'`` resets to the full bundle (None). + effective_skill_subset = effective_deploy_skill_subset( + skill_subset_from_cli=ctx.skill_subset_from_cli, + cli_subset=ctx.skill_subset, + persisted_subset=dep_ref.skill_subset, + ) + # When the additive union deploys more skills than the user named on + # this invocation, name the retained pins so the deployed set is not + # a silent surprise (verbose only -- the count already renders). + if logger and ctx.skill_subset and effective_skill_subset: + retained = sorted(set(effective_skill_subset) - set(ctx.skill_subset)) + if retained: + logger.verbose_detail( + f" [i] {dep_key}: retaining previously pinned " + f"skill(s): {', '.join(retained)}" + ) int_result = integrate_package_primitives( m.package_info, ctx.project_root, @@ -147,16 +168,7 @@ def _integrate_materialization( package_name=dep_key, logger=logger, scope=ctx.scope, - # Per-package effective subset: CLI --skill overrides per-entry - # apm.yml skills:. When CLI is absent (bare reinstall), fall back - # to the dep_ref's persisted skill_subset. - # When CLI explicitly provided (even --skill '*'), use ctx value - # (which is None for '*' = install all). - skill_subset=( - ctx.skill_subset - if ctx.skill_subset_from_cli - else (tuple(dep_ref.skill_subset) if dep_ref.skill_subset else None) - ), + skill_subset=effective_skill_subset, dep_target_subset=dep_ref.target_subset, ctx=ctx, allow_executables=_effective_allow(ctx), diff --git a/tests/unit/install/test_skill_subset_deploy_parity.py b/tests/unit/install/test_skill_subset_deploy_parity.py new file mode 100644 index 000000000..7dac966a2 --- /dev/null +++ b/tests/unit/install/test_skill_subset_deploy_parity.py @@ -0,0 +1,330 @@ +"""Regression tests for additive ``--skill`` *deployment* parity (issue #1786). + +PR #1786 made ``apm.yml``/lockfile persistence of ``--skill`` subsets additive, +but the deployment path kept using the raw CLI subset, so a second +``apm install --skill B`` deployed only ``B`` and the stale-file +cleanup deleted the previously deployed skill ``A`` from the target folder. + +These tests pin the additive *deployment* contract: + +* unit: ``effective_deploy_skill_subset`` union semantics + ``--skill '*'`` reset +* e2e: two atomic ``--skill`` installs leave BOTH skills on disk and in the + lockfile; ``--skill '*'`` deploys all; a manifest-driven reduction + cleans the dropped skill; uninstall removes the whole union. +""" + +from __future__ import annotations + +from pathlib import Path + +import yaml +from click.testing import CliRunner + +from apm_cli.install.package_resolution import ( + apply_cli_skill_pin, + cli_skill_subset, + effective_deploy_skill_subset, +) + +# --------------------------------------------------------------------------- +# Unit: the additive-union helper +# --------------------------------------------------------------------------- + + +class TestEffectiveDeploySkillSubset: + def test_bare_install_uses_persisted_union(self): + # No CLI --skill (bare reinstall): deploy whatever the manifest pins. + result = effective_deploy_skill_subset( + skill_subset_from_cli=False, + cli_subset=None, + persisted_subset=["alpha", "beta"], + ) + assert result == ("alpha", "beta") + + def test_bare_install_no_subset_returns_none(self): + result = effective_deploy_skill_subset( + skill_subset_from_cli=False, + cli_subset=None, + persisted_subset=None, + ) + assert result is None + + def test_additive_cli_unions_with_persisted(self): + # `apm install bundle --skill beta` when manifest already pins alpha: + # deploy BOTH (this is the bug the fix closes). + result = effective_deploy_skill_subset( + skill_subset_from_cli=True, + cli_subset=("beta",), + persisted_subset=["alpha", "beta"], + ) + assert result == ("alpha", "beta") + + def test_additive_cli_includes_requested_even_if_manifest_stale(self): + # Safety net: the just-requested CLI skill is always deployed even if + # the persisted subset has not yet caught up. + result = effective_deploy_skill_subset( + skill_subset_from_cli=True, + cli_subset=("gamma",), + persisted_subset=["alpha"], + ) + assert result == ("alpha", "gamma") + + def test_star_reset_returns_none(self): + # `--skill '*'` => from_cli True with empty CLI subset => deploy all. + result = effective_deploy_skill_subset( + skill_subset_from_cli=True, + cli_subset=None, + persisted_subset=["alpha", "beta"], + ) + assert result is None + + def test_result_is_sorted_and_deduped(self): + result = effective_deploy_skill_subset( + skill_subset_from_cli=True, + cli_subset=("beta", "beta"), + persisted_subset=["alpha", "beta"], + ) + assert result == ("alpha", "beta") + + +# --------------------------------------------------------------------------- +# Unit: the CLI-subset normalizer + skill-pin applier +# --------------------------------------------------------------------------- + + +class TestCliSkillSubset: + def test_absent_returns_none(self): + assert cli_skill_subset(()) is None + + def test_star_returns_none(self): + # `--skill '*'` collapses to None (all skills); origin tracked separately. + assert cli_skill_subset(("*",)) is None + + def test_named_returns_tuple(self): + assert cli_skill_subset(("beta",)) == ("beta",) + + def test_star_mixed_with_names_still_resets(self): + # Any '*' present means "all skills", even alongside named skills. + assert cli_skill_subset(("alpha", "*")) is None + + +class _FakeDepRef: + """Minimal stand-in exercising apply_cli_skill_pin's reset/no-op branches.""" + + def __init__(self, identity, canonical, yml_entry): + self._identity = identity + self._canonical = canonical + self._yml_entry = yml_entry + self.skill_subset = ["stale"] + + def get_identity(self): + return self._identity + + def to_canonical(self): + return self._canonical + + def to_apm_yml_entry(self): + return self._yml_entry + + +class TestApplyCliSkillPin: + def test_star_reset_clears_pin_and_records_full_entry(self): + # `--skill '*'`: clear the pin and write the refreshed full-bundle entry. + dep_ref = _FakeDepRef("owner/bundle", "owner/bundle@main", "owner/bundle") + entries: dict = {} + apply_cli_skill_pin( + dep_ref, + None, + True, + [], + entries, + dependency_reference_cls=object, + ) + assert dep_ref.skill_subset is None + assert entries == {"owner/bundle@main": "owner/bundle"} + + def test_no_cli_flag_is_noop(self): + # Bare install (flag absent): leave the pin and apm.yml entries untouched. + dep_ref = _FakeDepRef("owner/bundle", "owner/bundle@main", "owner/bundle") + entries: dict = {} + apply_cli_skill_pin( + dep_ref, + None, + False, + [], + entries, + dependency_reference_cls=object, + ) + assert dep_ref.skill_subset == ["stale"] + assert entries == {} + + +# --------------------------------------------------------------------------- +# E2E fixtures: a local directory skill bundle + a consumer project +# --------------------------------------------------------------------------- + + +def _make_skill_bundle(base: Path, skills: tuple[str, ...]) -> Path: + bundle = base / "azure-skills" + for name in skills: + skill_dir = bundle / "skills" / name + skill_dir.mkdir(parents=True, exist_ok=True) + (skill_dir / "SKILL.md").write_bytes( + ( + f"---\nname: {name}\ndescription: Demo skill {name} for additive " + f"deploy parity.\n---\n# {name}\nBody for {name}.\n" + ).encode() + ) + (bundle / "apm.yml").write_bytes( + yaml.dump({"name": "azure-skills", "version": "1.0.0"}).encode() + ) + return bundle + + +def _make_project(base: Path) -> Path: + project = base / "project" + project.mkdir(parents=True, exist_ok=True) + (project / "apm.yml").write_bytes(yaml.dump({"name": "consumer", "version": "1.0.0"}).encode()) + return project + + +def _install(project: Path, monkeypatch, *args: str): + from apm_cli.cli import cli + from apm_cli.models.apm_package import clear_apm_yml_cache + + # Each real `apm` command runs in a fresh process with an empty parse + # cache. CliRunner reuses this process, so clear the module-level + # apm.yml cache to mirror production isolation (see clear_apm_yml_cache + # docstring -- "Call in tests for isolation"). + clear_apm_yml_cache() + monkeypatch.chdir(project) + return CliRunner().invoke(cli, ["install", *args], catch_exceptions=False) + + +def _deployed_skill_names(project: Path) -> set[str]: + skills_root = project / ".agents" / "skills" + if not skills_root.is_dir(): + return set() + return {p.name for p in skills_root.iterdir() if (p / "SKILL.md").exists()} + + +def _lock_skill_subset(project: Path) -> list[str]: + lock = project / "apm.lock.yaml" + if not lock.exists(): + return [] + data = yaml.safe_load(lock.read_text()) or {} + deps = data.get("dependencies", []) + # Lockfile serializes dependencies as a list of entries. + entries = deps.values() if isinstance(deps, dict) else deps + for dep in entries: + if isinstance(dep, dict) and dep.get("package_type") == "skill_bundle": + return sorted(dep.get("skill_subset") or []) + return [] + + +# --------------------------------------------------------------------------- +# E2E: the core additive-deployment regression +# --------------------------------------------------------------------------- + + +class TestAdditiveSkillDeployment: + def test_second_skill_install_deploys_on_top_without_erasing_first(self, tmp_path, monkeypatch): + bundle = _make_skill_bundle(tmp_path / "src", ("cosmos-db", "functions", "aks")) + project = _make_project(tmp_path / "dst") + + r1 = _install( + project, monkeypatch, str(bundle), "--skill", "cosmos-db", "--target", "agent-skills" + ) + assert r1.exit_code == 0, r1.output + assert _deployed_skill_names(project) == {"cosmos-db"} + + r2 = _install( + project, monkeypatch, str(bundle), "--skill", "functions", "--target", "agent-skills" + ) + assert r2.exit_code == 0, r2.output + + # THE FIX: functions deployed ON TOP of cosmos-db, not replacing it. + assert _deployed_skill_names(project) == {"cosmos-db", "functions"} + + # apm.yml persists the union (pre-existing #1786 behavior). + manifest = yaml.safe_load((project / "apm.yml").read_text()) + entry = manifest["dependencies"]["apm"][0] + assert sorted(entry["skills"]) == ["cosmos-db", "functions"] + + # Lockfile reflects the union for both skill_subset and deployed_files. + assert _lock_skill_subset(project) == ["cosmos-db", "functions"] + + def test_bare_reinstall_keeps_full_union(self, tmp_path, monkeypatch): + bundle = _make_skill_bundle(tmp_path / "src", ("cosmos-db", "functions")) + project = _make_project(tmp_path / "dst") + _install( + project, monkeypatch, str(bundle), "--skill", "cosmos-db", "--target", "agent-skills" + ) + _install( + project, monkeypatch, str(bundle), "--skill", "functions", "--target", "agent-skills" + ) + + r = _install(project, monkeypatch, "--target", "agent-skills") + assert r.exit_code == 0, r.output + assert _deployed_skill_names(project) == {"cosmos-db", "functions"} + + def test_star_resets_to_all_skills(self, tmp_path, monkeypatch): + bundle = _make_skill_bundle(tmp_path / "src", ("cosmos-db", "functions", "aks")) + project = _make_project(tmp_path / "dst") + _install( + project, monkeypatch, str(bundle), "--skill", "cosmos-db", "--target", "agent-skills" + ) + + r = _install(project, monkeypatch, str(bundle), "--skill", "*", "--target", "agent-skills") + assert r.exit_code == 0, r.output + # '*' deploys the full bundle. + assert _deployed_skill_names(project) == {"cosmos-db", "functions", "aks"} + # Manifest reverts to the full bundle (no skills: pin). + manifest = yaml.safe_load((project / "apm.yml").read_text()) + entry = manifest["dependencies"]["apm"][0] + skills = entry.get("skills") if isinstance(entry, dict) else None + assert not skills + + def test_manifest_reduction_cleans_dropped_skill(self, tmp_path, monkeypatch): + bundle = _make_skill_bundle(tmp_path / "src", ("cosmos-db", "functions")) + project = _make_project(tmp_path / "dst") + _install( + project, monkeypatch, str(bundle), "--skill", "cosmos-db", "--target", "agent-skills" + ) + _install( + project, monkeypatch, str(bundle), "--skill", "functions", "--target", "agent-skills" + ) + assert _deployed_skill_names(project) == {"cosmos-db", "functions"} + + # Manually reduce the manifest to a single skill, then bare reinstall. + manifest_path = project / "apm.yml" + manifest = yaml.safe_load(manifest_path.read_text()) + manifest["dependencies"]["apm"][0]["skills"] = ["cosmos-db"] + manifest_path.write_bytes(yaml.dump(manifest).encode()) + + r = _install(project, monkeypatch, "--target", "agent-skills") + assert r.exit_code == 0, r.output + assert _deployed_skill_names(project) == {"cosmos-db"} + + def test_uninstall_removes_full_union(self, tmp_path, monkeypatch): + from apm_cli.cli import cli + + bundle = _make_skill_bundle(tmp_path / "src", ("cosmos-db", "functions")) + project = _make_project(tmp_path / "dst") + _install( + project, monkeypatch, str(bundle), "--skill", "cosmos-db", "--target", "agent-skills" + ) + _install( + project, monkeypatch, str(bundle), "--skill", "functions", "--target", "agent-skills" + ) + assert _deployed_skill_names(project) == {"cosmos-db", "functions"} + + monkeypatch.chdir(project) + # Local-path bundles are addressed by their path in apm.yml/uninstall. + from apm_cli.models.apm_package import clear_apm_yml_cache + + clear_apm_yml_cache() + r = CliRunner().invoke(cli, ["uninstall", str(bundle)], catch_exceptions=False) + assert r.exit_code == 0, r.output + # Whole bundle removed from disk -- deployed_files held the full union. + assert _deployed_skill_names(project) == set()