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/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index 49264459..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 = False + 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 = True + unpinned_count += 1 # Still need to integrate prompts for cached packages (zero-config behavior) if integrate_vscode or integrate_claude or integrate_opencode: @@ -1767,9 +1768,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 + unpinned_count += 1 # Collect for lockfile: get resolved commit and depth resolved_commit = None @@ -2199,8 +2200,12 @@ 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)") + if unpinned_count: + noun = "dependency has" if unpinned_count == 1 else "dependencies have" + diagnostics.info( + f"{unpinned_count} {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..e06b1771 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/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"], diff --git a/tests/unit/test_diagnostics.py b/tests/unit/test_diagnostics.py index 0c9d6198..314e8d62 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,74 @@ 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") + + 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" + ) 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