From 04f61c8d97333b0a1f2ebc6f64837e89af2e5ff0 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Tue, 17 Mar 2026 16:54:20 +0100 Subject: [PATCH 1/3] fix: move pinning hint to diagnostics section Replace inline _rich_info() tip with aggregated DiagnosticCollector info() emission. Unpinned deps are now counted (not just flagged) and rendered in the Diagnostics section at the bottom of install output, keeping the package tree clean. Changes: - Add CATEGORY_INFO to DiagnosticCollector with info() method and _render_info_group() renderer (blue [i] prefix) - Convert has_unpinned_deps bool to unpinned_count int in install.py - Emit single aggregated diagnostic emphasizing drift prevention - Remove TestUnpinnedDepsDetection (logic moved to diagnostics) - Add TestInfoCategory tests for the new info category Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/apm_cli/commands/install.py | 14 ++++++++----- src/apm_cli/utils/diagnostics.py | 22 ++++++++++++++++++++ tests/unit/test_diagnostics.py | 34 +++++++++++++++++++++++++++++++ tests/unit/test_install_output.py | 19 ----------------- 4 files changed, 65 insertions(+), 24 deletions(-) diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index 49264459..172dfbf6 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -1079,7 +1079,7 @@ def _collect_descendants(node, visited=None): # downloader already created above for transitive resolution installed_count = 0 - has_unpinned_deps = False + has_unpinned_deps = 0 # Phase 4 (#171): Parallel package downloads using ThreadPoolExecutor # Pre-download all non-cached packages in parallel for wall-clock speedup. @@ -1366,7 +1366,7 @@ def _collect_descendants(node, visited=None): _rich_info(f"✓ {display_name}{ref_str} (cached)") installed_count += 1 if not dep_ref.reference: - has_unpinned_deps = True + has_unpinned_deps += 1 # Still need to integrate prompts for cached packages (zero-config behavior) if integrate_vscode or integrate_claude or integrate_opencode: @@ -1767,9 +1767,9 @@ def _collect_descendants(node, visited=None): ref_suffix = f" @ {resolved}" if resolved else "" _rich_success(f"✓ {display_name}{ref_suffix}") - # Track whether any dep had no explicit ref (for hint) + # Track unpinned deps for aggregated diagnostic if not dep_ref.reference: - has_unpinned_deps = True + has_unpinned_deps += 1 # Collect for lockfile: get resolved commit and depth resolved_commit = None @@ -2200,7 +2200,11 @@ def _collect_descendants(node, visited=None): _rich_success(f"Installed {installed_count} APM dependencies") if has_unpinned_deps: - _rich_info("Tip: Pin versions with #tag or #sha for reproducible installs (e.g. owner/repo#v1.0.0)") + noun = "dependency has" if has_unpinned_deps == 1 else "dependencies have" + diagnostics.info( + f"{has_unpinned_deps} {noun} no pinned version " + f"-- pin with #tag or #sha to prevent drift" + ) return installed_count, total_prompts_integrated, total_agents_integrated, diagnostics diff --git a/src/apm_cli/utils/diagnostics.py b/src/apm_cli/utils/diagnostics.py index 270931e7..a9a1e1c1 100644 --- a/src/apm_cli/utils/diagnostics.py +++ b/src/apm_cli/utils/diagnostics.py @@ -24,6 +24,7 @@ CATEGORY_WARNING = "warning" CATEGORY_ERROR = "error" CATEGORY_SECURITY = "security" +CATEGORY_INFO = "info" _CATEGORY_ORDER = [ CATEGORY_SECURITY, @@ -31,6 +32,7 @@ CATEGORY_OVERWRITE, CATEGORY_WARNING, CATEGORY_ERROR, + CATEGORY_INFO, ] @@ -128,6 +130,18 @@ def security( ) ) + def info(self, message: str, package: str = "", detail: str = "") -> None: + """Record an informational hint (non-blocking, actionable guidance).""" + with self._lock: + self._diagnostics.append( + Diagnostic( + message=message, + category=CATEGORY_INFO, + package=package, + detail=detail, + ) + ) + # ------------------------------------------------------------------ # Query helpers # ------------------------------------------------------------------ @@ -204,6 +218,8 @@ def render_summary(self) -> None: self._render_warning_group(items) elif cat == CATEGORY_ERROR: self._render_error_group(items) + elif cat == CATEGORY_INFO: + self._render_info_group(items) if console: try: @@ -308,6 +324,12 @@ def _render_error_group(self, items: List[Diagnostic]) -> None: if d.detail and self.verbose: _rich_echo(f" {d.detail}", color="dim") + def _render_info_group(self, items: List[Diagnostic]) -> None: + for d in items: + _rich_info(f" [i] {d.message}") + if d.detail and self.verbose: + _rich_echo(f" └─ {d.detail}", color="dim") + def _group_by_package(items: List[Diagnostic]) -> Dict[str, List[Diagnostic]]: """Group diagnostics by package, preserving insertion order. diff --git a/tests/unit/test_diagnostics.py b/tests/unit/test_diagnostics.py index 0c9d6198..414391a2 100644 --- a/tests/unit/test_diagnostics.py +++ b/tests/unit/test_diagnostics.py @@ -9,6 +9,7 @@ from apm_cli.utils.diagnostics import ( CATEGORY_COLLISION, CATEGORY_ERROR, + CATEGORY_INFO, CATEGORY_OVERWRITE, CATEGORY_WARNING, Diagnostic, @@ -358,3 +359,36 @@ def test_preserves_insertion_order(self): ] groups = _group_by_package(items) assert list(groups.keys()) == ["z", "a", "m"] + + +# ── Info category ─────────────────────────────────────────────────── + + +class TestInfoCategory: + def test_info_adds_diagnostic(self): + dc = DiagnosticCollector() + dc.info("3 dependencies have no pinned version") + assert dc.has_diagnostics is True + assert len(dc._diagnostics) == 1 + assert dc._diagnostics[0].category == CATEGORY_INFO + assert dc._diagnostics[0].message == "3 dependencies have no pinned version" + + def test_info_renders_in_summary(self): + dc = DiagnosticCollector() + dc.info("2 dependencies have no pinned version -- pin with #tag") + with patch(f"{_MOCK_BASE}._get_console", return_value=None), \ + patch(f"{_MOCK_BASE}._rich_echo") as mock_echo, \ + patch(f"{_MOCK_BASE}._rich_warning"), \ + patch(f"{_MOCK_BASE}._rich_info") as mock_info: + dc.render_summary() + mock_info.assert_any_call( + " [i] 2 dependencies have no pinned version -- pin with #tag" + ) + + def test_info_appears_after_other_categories(self): + dc = DiagnosticCollector() + dc.info("hint message") + dc.warn("a warning", package="pkg") + # Info should still render (and after warnings) + assert dc.has_diagnostics is True + assert len(dc._diagnostics) == 2 diff --git a/tests/unit/test_install_output.py b/tests/unit/test_install_output.py index fe445a9f..6878fef1 100644 --- a/tests/unit/test_install_output.py +++ b/tests/unit/test_install_output.py @@ -93,22 +93,3 @@ def test_cached_lockfile_marked_as_cached(self): locked = MagicMock(resolved_commit="cached") result = self._format_cached_ref(dep, locked) assert result == " @ v2.0" - - -class TestUnpinnedDepsDetection: - """Test the has_unpinned_deps tracking logic.""" - - def test_dep_with_ref_is_pinned(self): - """Dependencies with explicit ref are not unpinned.""" - dep = DependencyReference.parse("owner/repo#v1.0.0") - assert dep.reference is not None # has_unpinned_deps would NOT be set - - def test_dep_without_ref_is_unpinned(self): - """Dependencies without explicit ref are unpinned.""" - dep = DependencyReference.parse("owner/repo") - assert dep.reference is None # has_unpinned_deps WOULD be set - - def test_branch_ref_is_still_pinned_for_hint(self): - """Branch refs count as 'pinned' for the hint (user explicitly chose a ref).""" - dep = DependencyReference.parse("owner/repo#main") - assert dep.reference == "main" # has_unpinned_deps would NOT be set From 63d07347c69d1e7e20797c5b3a03bf3a333356e0 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Tue, 17 Mar 2026 17:17:00 +0100 Subject: [PATCH 2/3] fix tests --- src/apm_cli/adapters/client/codex.py | 20 +++++++++++++++++--- tests/integration/test_local_install.py | 4 ++-- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/apm_cli/adapters/client/codex.py b/src/apm_cli/adapters/client/codex.py index 4f8b7fc0..fe7da478 100644 --- a/src/apm_cli/adapters/client/codex.py +++ b/src/apm_cli/adapters/client/codex.py @@ -218,10 +218,24 @@ def _format_server_config(self, server_info, env_overrides=None, runtime_vars=No # Generate command and args based on package type if registry_name == "npm": config["command"] = runtime_hint or "npx" - # Always include package name; filter duplicates from legacy runtime_arguments all_args = processed_runtime_args + processed_package_args - extra_args = [a for a in all_args if a != package_name] if all_args else [] - config["args"] = ["-y", package_name] + extra_args + if all_args: + # If runtime_arguments already include the package (bare or + # versioned), use them as-is — they are authoritative from + # the registry and may carry a version pin. + has_pkg = any( + a == package_name or a.startswith(f"{package_name}@") + for a in all_args + ) + if has_pkg: + config["args"] = all_args + else: + # Legacy: runtime_arguments don't mention the package, + # prepend -y + bare name ourselves. + extra_args = [a for a in all_args if a != "-y"] + config["args"] = ["-y", package_name] + extra_args + else: + config["args"] = ["-y", package_name] # For NPM packages, also use env block for environment variables if resolved_env: config["env"] = resolved_env diff --git a/tests/integration/test_local_install.py b/tests/integration/test_local_install.py index fc583970..31d54db8 100644 --- a/tests/integration/test_local_install.py +++ b/tests/integration/test_local_install.py @@ -134,7 +134,7 @@ def test_install_local_package_relative_path(self, temp_workspace, apm_command): assert (install_dir / ".apm" / "instructions" / "test-skill.instructions.md").exists() # Verify lockfile - lock_path = consumer / "apm.lock" + lock_path = consumer / "apm.lock.yaml" assert lock_path.exists(), "Lockfile not created" with open(lock_path) as f: lock_data = yaml.safe_load(f) @@ -365,7 +365,7 @@ def test_pack_rejects_with_local_deps(self, temp_workspace, apm_command): source="local", local_path="../packages/local-skills", )) - _lock.write(consumer / "apm.lock") + _lock.write(consumer / "apm.lock.yaml") result = subprocess.run( [apm_command, "pack"], From 8d400aaa2043325dc505147fc10ceca52d89d233 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Tue, 17 Mar 2026 18:01:10 +0100 Subject: [PATCH 3/3] fix: address PR review comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename has_unpinned_deps → unpinned_count for clarity - Fix double blank line before diagnostics header - Align info detail indentation (6→4 spaces) with warnings - Make test_info_appears_after_other_categories actually verify render order - Add tests for unpinned deps message formatting (singular/plural) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/apm_cli/commands/install.py | 15 ++++++----- src/apm_cli/utils/diagnostics.py | 2 +- tests/unit/test_diagnostics.py | 44 +++++++++++++++++++++++++++++--- 3 files changed, 50 insertions(+), 11 deletions(-) diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index 172dfbf6..457c7c9f 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -484,9 +484,10 @@ def install(ctx, packages, runtime, exclude, only, update, dry_run, force, verbo MCPIntegrator.update_lockfile(old_mcp_servers, mcp_configs=old_mcp_configs) # Show beautiful post-install summary - _rich_blank_line() if apm_diagnostics and apm_diagnostics.has_diagnostics: apm_diagnostics.render_summary() + else: + _rich_blank_line() if not only: # Load apm.yml config for summary apm_config = _load_apm_config() @@ -1079,7 +1080,7 @@ def _collect_descendants(node, visited=None): # downloader already created above for transitive resolution installed_count = 0 - has_unpinned_deps = 0 + unpinned_count = 0 # Phase 4 (#171): Parallel package downloads using ThreadPoolExecutor # Pre-download all non-cached packages in parallel for wall-clock speedup. @@ -1366,7 +1367,7 @@ def _collect_descendants(node, visited=None): _rich_info(f"✓ {display_name}{ref_str} (cached)") installed_count += 1 if not dep_ref.reference: - has_unpinned_deps += 1 + unpinned_count += 1 # Still need to integrate prompts for cached packages (zero-config behavior) if integrate_vscode or integrate_claude or integrate_opencode: @@ -1769,7 +1770,7 @@ def _collect_descendants(node, visited=None): # Track unpinned deps for aggregated diagnostic if not dep_ref.reference: - has_unpinned_deps += 1 + unpinned_count += 1 # Collect for lockfile: get resolved commit and depth resolved_commit = None @@ -2199,10 +2200,10 @@ def _collect_descendants(node, visited=None): _rich_success(f"Installed {installed_count} APM dependencies") - if has_unpinned_deps: - noun = "dependency has" if has_unpinned_deps == 1 else "dependencies have" + if unpinned_count: + noun = "dependency has" if unpinned_count == 1 else "dependencies have" diagnostics.info( - f"{has_unpinned_deps} {noun} no pinned version " + f"{unpinned_count} {noun} no pinned version " f"-- pin with #tag or #sha to prevent drift" ) diff --git a/src/apm_cli/utils/diagnostics.py b/src/apm_cli/utils/diagnostics.py index a9a1e1c1..e06b1771 100644 --- a/src/apm_cli/utils/diagnostics.py +++ b/src/apm_cli/utils/diagnostics.py @@ -328,7 +328,7 @@ def _render_info_group(self, items: List[Diagnostic]) -> None: for d in items: _rich_info(f" [i] {d.message}") if d.detail and self.verbose: - _rich_echo(f" └─ {d.detail}", color="dim") + _rich_echo(f" └─ {d.detail}", color="dim") def _group_by_package(items: List[Diagnostic]) -> Dict[str, List[Diagnostic]]: diff --git a/tests/unit/test_diagnostics.py b/tests/unit/test_diagnostics.py index 414391a2..314e8d62 100644 --- a/tests/unit/test_diagnostics.py +++ b/tests/unit/test_diagnostics.py @@ -389,6 +389,44 @@ def test_info_appears_after_other_categories(self): dc = DiagnosticCollector() dc.info("hint message") dc.warn("a warning", package="pkg") - # Info should still render (and after warnings) - assert dc.has_diagnostics is True - assert len(dc._diagnostics) == 2 + + call_order = [] + with patch(f"{_MOCK_BASE}._get_console", return_value=None), \ + patch(f"{_MOCK_BASE}._rich_echo") as mock_echo, \ + patch(f"{_MOCK_BASE}._rich_warning", side_effect=lambda *a, **k: call_order.append("warning")), \ + patch(f"{_MOCK_BASE}._rich_info", side_effect=lambda *a, **k: call_order.append("info")): + dc.render_summary() + # Warning must render before info + warn_idx = next(i for i, c in enumerate(call_order) if c == "warning") + info_idx = next(i for i, c in enumerate(call_order) if c == "info") + assert warn_idx < info_idx, f"warning at {warn_idx} should precede info at {info_idx}" + + def test_info_unpinned_deps_singular(self): + dc = DiagnosticCollector() + dc.info( + "1 dependency has no pinned version " + "-- pin with #tag or #sha to prevent drift" + ) + with patch(f"{_MOCK_BASE}._get_console", return_value=None), \ + patch(f"{_MOCK_BASE}._rich_echo"), \ + patch(f"{_MOCK_BASE}._rich_info") as mock_info: + dc.render_summary() + mock_info.assert_any_call( + " [i] 1 dependency has no pinned version " + "-- pin with #tag or #sha to prevent drift" + ) + + def test_info_unpinned_deps_plural(self): + dc = DiagnosticCollector() + dc.info( + "3 dependencies have no pinned version " + "-- pin with #tag or #sha to prevent drift" + ) + with patch(f"{_MOCK_BASE}._get_console", return_value=None), \ + patch(f"{_MOCK_BASE}._rich_echo"), \ + patch(f"{_MOCK_BASE}._rich_info") as mock_info: + dc.render_summary() + mock_info.assert_any_call( + " [i] 3 dependencies have no pinned version " + "-- pin with #tag or #sha to prevent drift" + )