Fix --skill to merge additively with existing skills in apm.yml

src/apm_cli/commands/install.py

@@ -48,6 +48,7 @@
 from apm_cli.install.package_resolution import (
     GIT_PARENT_USER_SCOPE_ERROR,
     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,
@@ -462,18 +463,16 @@ 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
+            # 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:
-                # Normalize: strip whitespace, drop empty strings, deduplicate
-                # (preserve order) so invalid or redundant names can't persist.
-                _seen: builtins.set[str] = builtins.set()
-                _normalized: builtins.list[str] = []
-                for _s in skill_subset:
-                    _s = _s.strip()
-                    if _s and _s not in _seen:
-                        _seen.add(_s)
-                        _normalized.append(_s)
-                dep_ref.skill_subset = _normalized
+                dep_ref.skill_subset = normalize_and_merge_skill_subset(
+                    skill_subset,
+                    current_deps,
+                    identity,
+                    dependency_reference_cls=DependencyReference,
+                )
             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:

src/apm_cli/install/package_resolution.py

@@ -129,6 +129,57 @@ def user_scope_rejection_reason(dep_ref: Any, scope: Any) -> str | None:
     return None
 
 
+def get_existing_skill_subset(
+    current_deps: builtins.list,
+    identity: str,
+    *,
+    dependency_reference_cls: Any,
+) -> builtins.list[str] | None:
+    """Return the persisted ``skills:`` list for *identity*, or None."""
+    for dep_entry in current_deps:
+        try:
+            if isinstance(dep_entry, builtins.str):
+                existing_ref = dependency_reference_cls.parse(dep_entry)
+            elif isinstance(dep_entry, builtins.dict):
+                existing_ref = dependency_reference_cls.parse_from_dict(dep_entry)
+            else:
+                continue
+        except (ValueError, TypeError, AttributeError, KeyError):
+            continue
+        if existing_ref.get_identity() == identity:
+            subset = getattr(existing_ref, "skill_subset", None)
+            return list(subset) if subset else None
+    return None
+
+
+def normalize_and_merge_skill_subset(
+    cli_subset: builtins.tuple[str, ...],
+    current_deps: builtins.list,
+    identity: str,
+    *,
+    dependency_reference_cls: Any,
+) -> builtins.list[str]:
+    """Normalize CLI ``--skill`` names and merge with existing manifest skills.
+
+    Strips whitespace, drops empty strings, deduplicates, then unions with
+    the persisted ``skills:`` list from ``apm.yml`` so that repeated
+    ``--skill`` invocations are additive (issue #1771).
+
+    Returns a sorted, deduplicated list ready for ``dep_ref.skill_subset``.
+    """
+    seen: builtins.set[str] = builtins.set()
+    for s in cli_subset:
+        s = s.strip()
+        if s:
+            seen.add(s)
+    existing = get_existing_skill_subset(
+        current_deps, identity, dependency_reference_cls=dependency_reference_cls
+    )
+    if existing:
+        seen.update(existing)
+    return sorted(seen)
+
+
 def manifest_has_different_entry_for_identity(
     current_deps: builtins.list,
     identity: str,

tests/unit/commands/test_install_skill_subset.py

@@ -131,12 +131,12 @@ def test_skill_subset_rejects_empty_strings(tmp_path):
 
 
 def test_skill_subset_deduplicates_preserving_order(tmp_path):
-    """Duplicate skill names are removed; first occurrence order is kept."""
+    """Duplicate skill names are removed; result is sorted."""
     apm_yml = tmp_path / "apm.yml"
     _make_apm_yml(apm_yml)
 
     dep_ref = _run_validate(
         apm_yml, skill_subset=("skill-b", "skill-a", "skill-b", "skill-a", "skill-c")
     )
 
-    assert dep_ref.skill_subset == ["skill-b", "skill-a", "skill-c"]
+    assert dep_ref.skill_subset == ["skill-a", "skill-b", "skill-c"]

tests/unit/test_skill_subset_persistence.py

@@ -456,3 +456,170 @@ def test_both_empty_passes(self):
 
         result = _check_skill_subset_consistency(manifest, lock)
         assert result.passed is True
+
+
+# ============================================================================
+# get_existing_skill_subset — read persisted skills: from current_deps
+# ============================================================================
+
+
+class TestGetExistingSkillSubset:
+    """Test get_existing_skill_subset helper (issue #1771)."""
+
+    def test_returns_skills_from_dict_entry(self):
+        """Dict entry with skills: returns the list."""
+        from apm_cli.install.package_resolution import get_existing_skill_subset
+
+        current_deps = [{"git": "owner/repo", "skills": ["alpha", "beta"]}]
+        result = get_existing_skill_subset(
+            current_deps, "owner/repo", dependency_reference_cls=DependencyReference
+        )
+        assert result == ["alpha", "beta"]
+
+    def test_returns_none_for_string_entry(self):
+        """Plain string entry has no skill_subset -- returns None."""
+        from apm_cli.install.package_resolution import get_existing_skill_subset
+
+        current_deps = ["owner/repo"]
+        result = get_existing_skill_subset(
+            current_deps, "owner/repo", dependency_reference_cls=DependencyReference
+        )
+        assert result is None
+
+    def test_returns_none_when_identity_not_found(self):
+        """No matching identity returns None."""
+        from apm_cli.install.package_resolution import get_existing_skill_subset
+
+        current_deps = [{"git": "other/repo", "skills": ["alpha"]}]
+        result = get_existing_skill_subset(
+            current_deps, "owner/repo", dependency_reference_cls=DependencyReference
+        )
+        assert result is None
+
+    def test_returns_none_for_empty_deps(self):
+        """Empty deps list returns None."""
+        from apm_cli.install.package_resolution import get_existing_skill_subset
+
+        result = get_existing_skill_subset(
+            [], "owner/repo", dependency_reference_cls=DependencyReference
+        )
+        assert result is None
+
+    def test_skips_unparseable_entries(self):
+        """Unparseable entries are silently skipped."""
+        from apm_cli.install.package_resolution import get_existing_skill_subset
+
+        current_deps = [42, {"git": "owner/repo", "skills": ["alpha"]}]
+        result = get_existing_skill_subset(
+            current_deps, "owner/repo", dependency_reference_cls=DependencyReference
+        )
+        assert result == ["alpha"]
+
+
+# ============================================================================
+# _resolve_package_references — additive --skill merge (issue #1771)
+# ============================================================================
+
+
+class TestSkillSubsetAdditiveMerge:
+    """Verify that --skill merges with existing manifest skills: list."""
+
+    def test_skill_names_merge_with_existing(self):
+        """New --skill names are merged with the persisted skills: list."""
+        from unittest.mock import patch
+
+        from apm_cli.commands.install import _resolve_package_references
+
+        current_deps = [{"git": "owner/repo", "skills": ["grill-me"]}]
+        existing_identities = {"owner/repo"}
+
+        with (
+            patch(
+                "apm_cli.commands.install._validate_package_exists",
+                return_value=True,
+            ),
+        ):
+            (
+                valid_outcomes,
+                _invalid,
+                _validated,
+                _mktplace,
+                apm_yml_entries,
+                _changed,
+            ) = _resolve_package_references(
+                ["owner/repo"],
+                current_deps,
+                existing_identities.copy(),
+                skill_subset=("qa",),
+            )
+
+        # The merged skill list should contain both the existing and new
+        assert len(valid_outcomes) == 1
+        assert "owner/repo" in apm_yml_entries
+        entry = apm_yml_entries["owner/repo"]
+        assert isinstance(entry, dict)
+        assert sorted(entry["skills"]) == ["grill-me", "qa"]
+
+    def test_skill_names_no_duplicates(self):
+        """Re-adding an existing skill name does not produce duplicates."""
+        from unittest.mock import patch
+
+        from apm_cli.commands.install import _resolve_package_references
+
+        current_deps = [{"git": "owner/repo", "skills": ["alpha", "beta"]}]
+        existing_identities = {"owner/repo"}
+
+        with (
+            patch(
+                "apm_cli.commands.install._validate_package_exists",
+                return_value=True,
+            ),
+        ):
+            (
+                _valid,
+                _invalid,
+                _validated,
+                _mktplace,
+                apm_yml_entries,
+                _changed,
+            ) = _resolve_package_references(
+                ["owner/repo"],
+                current_deps,
+                existing_identities.copy(),
+                skill_subset=("alpha", "gamma"),
+            )
+
+        entry = apm_yml_entries["owner/repo"]
+        assert sorted(entry["skills"]) == ["alpha", "beta", "gamma"]
+
+    def test_skill_new_package_no_existing_to_merge(self):
+        """First install with --skill on a new package works (no existing)."""
+        from unittest.mock import patch
+
+        from apm_cli.commands.install import _resolve_package_references
+
+        current_deps = []
+        existing_identities: set[str] = set()
+
+        with (
+            patch(
+                "apm_cli.commands.install._validate_package_exists",
+                return_value=True,
+            ),
+        ):
+            (
+                _valid,
+                _invalid,
+                _validated,
+                _mktplace,
+                apm_yml_entries,
+                _changed,
+            ) = _resolve_package_references(
+                ["owner/repo"],
+                current_deps,
+                existing_identities.copy(),
+                skill_subset=("qa",),
+            )
+
+        entry = apm_yml_entries["owner/repo"]
+        assert entry["skills"] == ["qa"]