diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index d47abf82e..4173161a0 100644 --- a/src/apm_cli/commands/install.py +++ b/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: diff --git a/src/apm_cli/install/package_resolution.py b/src/apm_cli/install/package_resolution.py index c95aea0a1..52b4ad5d8 100644 --- a/src/apm_cli/install/package_resolution.py +++ b/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, diff --git a/tests/unit/commands/test_install_skill_subset.py b/tests/unit/commands/test_install_skill_subset.py index 6066934f1..6b847aacf 100644 --- a/tests/unit/commands/test_install_skill_subset.py +++ b/tests/unit/commands/test_install_skill_subset.py @@ -131,7 +131,7 @@ 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) @@ -139,4 +139,4 @@ def test_skill_subset_deduplicates_preserving_order(tmp_path): 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"] diff --git a/tests/unit/test_skill_subset_persistence.py b/tests/unit/test_skill_subset_persistence.py index f4c7f1499..cdb551696 100644 --- a/tests/unit/test_skill_subset_persistence.py +++ b/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"]