diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d31be1a8..2e29bfd77 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `apm install -g #` now updates an existing unpinned global dependency entry instead of leaving the manifest floating. (#1559) - `apm install` now honors manifest `targets:` without falling back to the legacy Copilot target when singular `target:` is absent. (#1560) ## [0.16.0] - 2026-05-28 diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index fd7c34220..d235cd726 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -48,9 +48,9 @@ from apm_cli.install.package_resolution import ( GIT_PARENT_USER_SCOPE_ERROR, dependency_reference_to_yaml_entry, - merge_structured_entry_into_current_deps, persist_dependency_list_if_changed, resolve_parsed_dependency_reference, + update_existing_dependency_entry_if_needed, user_scope_rejection_reason, ) @@ -536,24 +536,24 @@ def warning_handler(msg): logger=logger, dep_ref=dep_ref, ): + updates_existing_entry = update_existing_dependency_entry_if_needed( + current_deps, + already_in_deps=already_in_deps, + apm_yml_entries=_apm_yml_entries, + canonical=canonical, + dep_ref=dep_ref, + identity=identity, + dependency_reference_cls=DependencyReference, + logger=logger, + ) valid_outcomes.append((canonical, already_in_deps)) if logger: - logger.validation_pass(canonical, already_present=already_in_deps) + logger.validation_pass(canonical, already_in_deps, updates_existing_entry) if not already_in_deps: validated_packages.append(canonical) existing_identities.add(identity) # prevent duplicates within batch - elif canonical in _apm_yml_entries: - structured_entry = _apm_yml_entries[canonical] - merge_structured_entry_into_current_deps( - current_deps, - structured_entry, - identity, - canonical, - dependency_reference_cls=DependencyReference, - logger=logger, - ) - dependencies_changed = True + dependencies_changed = dependencies_changed or updates_existing_entry if marketplace_provenance: _marketplace_provenance[identity] = marketplace_provenance else: diff --git a/src/apm_cli/core/command_logger.py b/src/apm_cli/core/command_logger.py index 12f67fe83..f4617360b 100644 --- a/src/apm_cli/core/command_logger.py +++ b/src/apm_cli/core/command_logger.py @@ -218,9 +218,11 @@ def validation_start(self, count: int): noun = "package" if count == 1 else "packages" _rich_info(f"Validating {count} {noun}...", symbol="gear") - def validation_pass(self, canonical: str, already_present: bool): + def validation_pass(self, canonical: str, already_present: bool, updated: bool = False): """Log a package that passed validation.""" - if already_present: + if updated: + _rich_echo(f"{canonical} (updated ref in apm.yml)", color="dim", symbol="check") + elif already_present: _rich_echo(f"{canonical} (already in apm.yml)", color="dim", symbol="check") else: _rich_success(canonical, symbol="check") diff --git a/src/apm_cli/install/package_resolution.py b/src/apm_cli/install/package_resolution.py index e7ae2ef25..c95aea0a1 100644 --- a/src/apm_cli/install/package_resolution.py +++ b/src/apm_cli/install/package_resolution.py @@ -129,6 +129,65 @@ def user_scope_rejection_reason(dep_ref: Any, scope: Any) -> str | None: return None +def manifest_has_different_entry_for_identity( + current_deps: builtins.list, + identity: str, + canonical: str, + *, + dependency_reference_cls: Any, +) -> bool: + """Return True when apm.yml already has *identity* but not *canonical*.""" + 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: + return existing_ref.to_canonical() != canonical + return False + + +def update_existing_dependency_entry_if_needed( + current_deps: builtins.list, + *, + already_in_deps: bool, + apm_yml_entries: dict, + canonical: str, + dep_ref: Any, + identity: str, + dependency_reference_cls: Any, + logger: Any = None, +) -> bool: + """Rewrite an existing manifest dep when the requested ref changed.""" + should_update = already_in_deps and ( + canonical in apm_yml_entries + or ( + dep_ref.reference + and manifest_has_different_entry_for_identity( + current_deps, + identity, + canonical, + dependency_reference_cls=dependency_reference_cls, + ) + ) + ) + if should_update: + merge_structured_entry_into_current_deps( + current_deps, + apm_yml_entries.get(canonical, dep_ref.to_apm_yml_entry()), + identity, + canonical, + dependency_reference_cls=dependency_reference_cls, + logger=logger, + ) + return should_update + + def merge_structured_entry_into_current_deps( current_deps: builtins.list, structured_entry: dict, @@ -183,7 +242,7 @@ def persist_dependency_list_if_changed( dump_yaml(data, apm_yml_path) if logger: - logger.success(f"Updated {apm_yml_filename} to preserve marketplace subdirectory entry") + logger.success(f"Updated {apm_yml_filename} dependency entries") except Exception as e: if logger: logger.error(f"Failed to write {apm_yml_filename}: {e}") diff --git a/tests/unit/commands/test_install_resolve_refs.py b/tests/unit/commands/test_install_resolve_refs.py index 7489afd52..ea159d978 100644 --- a/tests/unit/commands/test_install_resolve_refs.py +++ b/tests/unit/commands/test_install_resolve_refs.py @@ -15,7 +15,7 @@ from unittest.mock import MagicMock, patch # The function under test lives in the commands module. -from apm_cli.commands.install import _resolve_package_references +from apm_cli.commands.install import _check_package_conflicts, _resolve_package_references from apm_cli.models.apm_package import APMPackage, clear_apm_yml_cache from apm_cli.models.dependency.reference import DependencyReference @@ -165,6 +165,24 @@ def test_mixed_new_and_preexisting(self, mock_dep_cls, mock_validate): assert "github.com/owner/new-pkg" in existing assert len(existing) == 2 + @patch("apm_cli.commands.install._validate_package_exists", return_value=True) + def test_existing_unpinned_dependency_is_updated_when_cli_supplies_ref(self, mock_validate): + """An explicit CLI ref for an existing dep must replace the unpinned manifest entry.""" + current_deps = ["danielmeppiel/genesis"] + existing = _check_package_conflicts(current_deps) + + valid, invalid, validated, _mkt, _entries, changed = _resolve_package_references( + ["danielmeppiel/genesis#v0.4.0"], + current_deps, + existing, + ) + + assert invalid == [] + assert valid == [("danielmeppiel/genesis#v0.4.0", True)] + assert validated == [] + assert changed is True + assert current_deps == ["danielmeppiel/genesis#v0.4.0"] + class TestResolvePackageReferencesInvalidInput: """Invalid packages must not mutate the identity set.""" diff --git a/tests/unit/install/test_package_resolution_persistence.py b/tests/unit/install/test_package_resolution_persistence.py new file mode 100644 index 000000000..5382f87f6 --- /dev/null +++ b/tests/unit/install/test_package_resolution_persistence.py @@ -0,0 +1,30 @@ +"""Tests for dependency-list persistence messaging.""" + +from __future__ import annotations + +from unittest.mock import Mock, patch + +from apm_cli.install.package_resolution import persist_dependency_list_if_changed + + +def test_persist_dependency_list_reports_generic_manifest_update(): + """Manifest rewrites should not claim every change is marketplace-specific.""" + logger = Mock() + data = {"dependencies": {"apm": []}} + current_deps = ["danielmeppiel/genesis#v0.4.0"] + + with patch("apm_cli.utils.yaml_io.dump_yaml") as dump_yaml: + persist_dependency_list_if_changed( + dependencies_changed=True, + data=data, + dep_section="dependencies", + current_deps=current_deps, + apm_yml_path="apm.yml", + apm_yml_filename="apm.yml", + logger=logger, + rich_error=Mock(), + sys_exit=Mock(), + ) + + dump_yaml.assert_called_once_with(data, "apm.yml") + logger.success.assert_called_once_with("Updated apm.yml dependency entries") diff --git a/tests/unit/test_command_logger.py b/tests/unit/test_command_logger.py index ec8759800..0630677d3 100644 --- a/tests/unit/test_command_logger.py +++ b/tests/unit/test_command_logger.py @@ -197,6 +197,12 @@ def test_validation_pass_existing(self, mock_echo): logger.validation_pass("microsoft/repo", already_present=True) assert "already in apm.yml" in mock_echo.call_args[0][0] + @patch("apm_cli.core.command_logger._rich_echo") + def test_validation_pass_existing_updated(self, mock_echo): + logger = InstallLogger() + logger.validation_pass("microsoft/repo#v1", already_present=True, updated=True) + assert "updated ref in apm.yml" in mock_echo.call_args[0][0] + @patch("apm_cli.core.command_logger._rich_error") def test_validation_fail(self, mock_error): logger = InstallLogger()