Fix: make --skill deployment additive, not just persistence (#1786 follow-up)

CHANGELOG.md

@@ -7,6 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Fixed
+
+- `apm install <bundle> --skill X` is now properly additive: a later
+  `apm install <bundle> --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

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 <bundle> --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

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):

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` |

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:

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,

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():