From dd04d343aeb17c16c92e09a67a10fd0658c72576 Mon Sep 17 00:00:00 2001 From: Sergio Sisternes Date: Mon, 15 Jun 2026 18:22:30 +0200 Subject: [PATCH 1/2] Fix --skill to merge with existing skills instead of replacing When running `apm install repo --skill qa` on a package that already has `skills: [grill-me]` in apm.yml, the CLI now merges the new name into the existing list (producing `skills: [grill-me, qa]`) instead of replacing it. - Extract `normalize_and_merge_skill_subset()` helper into `package_resolution.py` to keep install.py within its line budget. - Add `get_existing_skill_subset()` to read persisted skills: from current_deps by identity. - Update test expectations: skill_subset is now always sorted (matches the existing `to_apm_yml_entry()` and `parse_from_dict()` contracts). - Add integration-level tests proving repeated --skill invocations are additive and that no duplicates are introduced. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/apm_cli/commands/install.py | 21 ++- src/apm_cli/install/package_resolution.py | 56 ++++++ .../commands/test_install_skill_subset.py | 2 +- tests/unit/test_skill_subset_persistence.py | 167 ++++++++++++++++++ 4 files changed, 234 insertions(+), 12 deletions(-) diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index ef3a2a0d8..49d8934a5 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, @@ -461,18 +462,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..d5e6c528f 100644 --- a/src/apm_cli/install/package_resolution.py +++ b/src/apm_cli/install/package_resolution.py @@ -129,6 +129,62 @@ 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() + normalized: builtins.list[str] = [] + for s in cli_subset: + s = s.strip() + if s and s not in seen: + seen.add(s) + normalized.append(s) + existing = get_existing_skill_subset( + current_deps, identity, dependency_reference_cls=dependency_reference_cls + ) + if existing: + for e in existing: + if e not in seen: + seen.add(e) + normalized.append(e) + 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..40c47d653 100644 --- a/tests/unit/commands/test_install_skill_subset.py +++ b/tests/unit/commands/test_install_skill_subset.py @@ -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"] From 995ecbbc366798cfcf77e01393c62eee2ae3ad80 Mon Sep 17 00:00:00 2001 From: Sergio Sisternes Date: Mon, 15 Jun 2026 23:32:31 +0200 Subject: [PATCH 2/2] Address review: remove unused normalized list, fix docstring - Remove the intermediate normalized list that was never used in the return value; build only the seen set and return sorted(seen). - Update test docstring to say 'result is sorted' instead of 'preserving order' since the behaviour is now sorted output. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/apm_cli/install/package_resolution.py | 9 ++------- tests/unit/commands/test_install_skill_subset.py | 2 +- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/src/apm_cli/install/package_resolution.py b/src/apm_cli/install/package_resolution.py index d5e6c528f..52b4ad5d8 100644 --- a/src/apm_cli/install/package_resolution.py +++ b/src/apm_cli/install/package_resolution.py @@ -168,20 +168,15 @@ def normalize_and_merge_skill_subset( Returns a sorted, deduplicated list ready for ``dep_ref.skill_subset``. """ seen: builtins.set[str] = builtins.set() - normalized: builtins.list[str] = [] for s in cli_subset: s = s.strip() - if s and s not in seen: + if s: seen.add(s) - normalized.append(s) existing = get_existing_skill_subset( current_deps, identity, dependency_reference_cls=dependency_reference_cls ) if existing: - for e in existing: - if e not in seen: - seen.add(e) - normalized.append(e) + seen.update(existing) return sorted(seen) diff --git a/tests/unit/commands/test_install_skill_subset.py b/tests/unit/commands/test_install_skill_subset.py index 40c47d653..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)