From b69d356129d3b481f87f6816a379c7ba2078e1e0 Mon Sep 17 00:00:00 2001 From: Travis Illig Date: Wed, 29 Apr 2026 10:48:17 -0700 Subject: [PATCH 01/22] fix(orphan): don't flag parent dirs of subdirectory packages as orphaned When a dependency uses dict-form (git: + path:) pointing at a nested subdirectory (e.g., .apm/skills/skill-name), the intermediate owner/repo/ directory contains .apm/ and is detected by _scan_installed_packages. The orphan check now recognizes these directories as ancestors of expected install paths rather than orphaned packages. The fix is applied consistently across all three orphan-detection sites: compile (_helpers.py), prune, and deps list. Co-Authored-By: Claude Opus 4.6 --- CHANGELOG.md | 1 + src/apm_cli/commands/_helpers.py | 6 ++- src/apm_cli/commands/deps/cli.py | 7 ++- src/apm_cli/commands/prune.py | 6 ++- tests/unit/test_command_helpers.py | 73 ++++++++++++++++++++++++++++++ 5 files changed, 90 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 38b11d843..9d1db2814 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `apm compile`, `apm prune`, and `apm deps list` no longer falsely flag parent directories of subdirectory virtual packages as orphaned. When a dependency uses dict-form (`git:` + `path:`) pointing at a nested path (e.g., `.apm/skills/skill-name`), the intermediate `owner/repo/` directory is now recognized as an ancestor of the expected install path rather than an orphaned package. - `apm install` and `apm compile` no longer exit 0 with success messages when `target:` in `apm.yml` is a CSV string -- the value now parses identically to the same input on `--target`, and zero-target resolution surfaces a warning instead of a silent no-op. (#820) - Remove redundant `seen` set from `_scan_patterns()` discovery walk (#918) - `apm pack` (marketplace producer) now respects `GITHUB_HOST` for GitHub Enterprise repos -- ref resolution, token lookup, and metadata fetch all use the configured host instead of hardcoded `github.com`. `git ls-remote` is authenticated so private repos work without separate credential setup. (#1008) diff --git a/src/apm_cli/commands/_helpers.py b/src/apm_cli/commands/_helpers.py index 9179e48d2..7f44b4557 100644 --- a/src/apm_cli/commands/_helpers.py +++ b/src/apm_cli/commands/_helpers.py @@ -209,7 +209,11 @@ def _check_orphaned_packages(): return [] installed = _scan_installed_packages(apm_modules_dir) - return [p for p in installed if p not in expected] + return [ + p for p in installed + if p not in expected + and not any(e.startswith(p + "/") for e in expected) + ] except Exception: return [] diff --git a/src/apm_cli/commands/deps/cli.py b/src/apm_cli/commands/deps/cli.py index 5385f234b..9de339974 100644 --- a/src/apm_cli/commands/deps/cli.py +++ b/src/apm_cli/commands/deps/cli.py @@ -176,7 +176,12 @@ def _resolve_scope_deps(apm_dir, logger, insecure_only=False): version = package.version or 'unknown' primitives = _count_primitives(candidate) - is_orphaned = org_repo_name not in declared_sources + is_orphaned = ( + org_repo_name not in declared_sources + and not any( + k.startswith(org_repo_name + "/") for k in declared_sources + ) + ) if is_orphaned: orphaned_packages.append(org_repo_name) diff --git a/src/apm_cli/commands/prune.py b/src/apm_cli/commands/prune.py index d52032827..f67315066 100644 --- a/src/apm_cli/commands/prune.py +++ b/src/apm_cli/commands/prune.py @@ -57,7 +57,11 @@ def prune(ctx, dry_run): sys.exit(1) installed_packages = _scan_installed_packages(apm_modules_dir) - orphaned_packages = [p for p in installed_packages if p not in expected_installed] + orphaned_packages = [ + p for p in installed_packages + if p not in expected_installed + and not any(e.startswith(p + "/") for e in expected_installed) + ] if not orphaned_packages: logger.success("No orphaned packages found. apm_modules/ is clean.", symbol="check") diff --git a/tests/unit/test_command_helpers.py b/tests/unit/test_command_helpers.py index 7f26705ca..2b32c8fd4 100644 --- a/tests/unit/test_command_helpers.py +++ b/tests/unit/test_command_helpers.py @@ -15,7 +15,9 @@ from apm_cli.commands._helpers import ( _atomic_write, + _build_expected_install_paths, _check_and_notify_updates, + _check_orphaned_packages, _get_default_script, _list_available_scripts, _load_apm_config, @@ -252,6 +254,77 @@ def test_returns_empty_for_empty_dir(self, tmp_path): assert result == [] +# --------------------------------------------------------------------------- +# _check_orphaned_packages — subdirectory ancestor false-positive fix +# --------------------------------------------------------------------------- + + +class TestCheckOrphanedPackagesSubdirectoryAncestor: + """Tests that parent directories of subdirectory virtual packages are not + falsely flagged as orphaned. + + When a subdirectory package is installed at owner/repo/.apm/skills/name, + the intermediate owner/repo/ directory contains .apm/ and gets picked up + by _scan_installed_packages. The orphan check must recognise it as an + ancestor of an expected path rather than an orphaned package. + """ + + def test_parent_of_subdirectory_package_not_orphaned(self, tmp_path, monkeypatch): + """owner/repo is not orphaned when owner/repo/.apm/skills/x is expected.""" + monkeypatch.chdir(tmp_path) + + # Set up apm.yml with a dict-form dependency using git: + path: + apm_yml = tmp_path / "apm.yml" + apm_yml.write_text( + "name: test\n" + "version: 1.0.0\n" + "dependencies:\n" + " apm:\n" + " - git: github.example.com/owner/repo\n" + " path: .apm/skills/my-skill\n", + encoding="utf-8", + ) + + # Simulate the on-disk layout: owner/repo/.apm/skills/my-skill/SKILL.md + apm_modules = tmp_path / "apm_modules" + skill_dir = apm_modules / "owner" / "repo" / ".apm" / "skills" / "my-skill" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text("# Skill") + + orphaned = _check_orphaned_packages() + assert orphaned == [], ( + f"Parent dir should not be orphaned; got: {orphaned}" + ) + + def test_actual_orphan_still_detected(self, tmp_path, monkeypatch): + """A truly orphaned package is still reported even with the ancestor fix.""" + monkeypatch.chdir(tmp_path) + + apm_yml = tmp_path / "apm.yml" + apm_yml.write_text( + "name: test\n" + "version: 1.0.0\n" + "dependencies:\n" + " apm:\n" + " - git: github.example.com/owner/repo\n" + " path: .apm/skills/my-skill\n", + encoding="utf-8", + ) + + # Simulate an additional unrelated package that IS orphaned + apm_modules = tmp_path / "apm_modules" + skill_dir = apm_modules / "owner" / "repo" / ".apm" / "skills" / "my-skill" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text("# Skill") + + orphan_dir = apm_modules / "other" / "stale-pkg" + orphan_dir.mkdir(parents=True) + (orphan_dir / "apm.yml").write_text("name: stale-pkg") + + orphaned = _check_orphaned_packages() + assert "other/stale-pkg" in orphaned + + # --------------------------------------------------------------------------- # _check_and_notify_updates # --------------------------------------------------------------------------- From 5f7f11b02682600fffb332dd97b9c8b7411118f7 Mon Sep 17 00:00:00 2001 From: Travis Illig Date: Wed, 29 Apr 2026 11:04:33 -0700 Subject: [PATCH 02/22] test(orphan): add coverage for whole-repo dependency not flagged as orphaned Ensures the ancestor-check fix does not regress orphan detection for standard owner/repo shorthand dependencies (the most common case). Co-Authored-By: Claude Opus 4.6 --- tests/unit/test_command_helpers.py | 59 ++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/tests/unit/test_command_helpers.py b/tests/unit/test_command_helpers.py index 2b32c8fd4..3dac30ac8 100644 --- a/tests/unit/test_command_helpers.py +++ b/tests/unit/test_command_helpers.py @@ -324,6 +324,65 @@ def test_actual_orphan_still_detected(self, tmp_path, monkeypatch): orphaned = _check_orphaned_packages() assert "other/stale-pkg" in orphaned + def test_whole_repo_dependency_not_orphaned(self, tmp_path, monkeypatch): + """A whole-repo dependency (owner/repo shorthand) is not flagged as orphaned.""" + monkeypatch.chdir(tmp_path) + + apm_yml = tmp_path / "apm.yml" + apm_yml.write_text( + "name: test\n" + "version: 1.0.0\n" + "dependencies:\n" + " apm:\n" + " - github.example.com/org/my-package\n", + encoding="utf-8", + ) + + # Simulate the installed whole-repo package with .apm content + apm_modules = tmp_path / "apm_modules" + pkg_dir = apm_modules / "org" / "my-package" + pkg_dir.mkdir(parents=True) + (pkg_dir / "apm.yml").write_text("name: my-package\nversion: 1.0.0") + apm_dir = pkg_dir / ".apm" + apm_dir.mkdir() + instr_dir = apm_dir / "instructions" + instr_dir.mkdir() + (instr_dir / "example.instructions.md").write_text("# Instructions") + + orphaned = _check_orphaned_packages() + assert orphaned == [], ( + f"Whole-repo package should not be orphaned; got: {orphaned}" + ) + + def test_whole_repo_with_unrelated_orphan(self, tmp_path, monkeypatch): + """Whole-repo dep is fine; an unrelated stale package IS orphaned.""" + monkeypatch.chdir(tmp_path) + + apm_yml = tmp_path / "apm.yml" + apm_yml.write_text( + "name: test\n" + "version: 1.0.0\n" + "dependencies:\n" + " apm:\n" + " - github.example.com/org/my-package\n", + encoding="utf-8", + ) + + apm_modules = tmp_path / "apm_modules" + # Declared package + pkg_dir = apm_modules / "org" / "my-package" + pkg_dir.mkdir(parents=True) + (pkg_dir / "apm.yml").write_text("name: my-package\nversion: 1.0.0") + + # Stale package not in apm.yml + stale_dir = apm_modules / "org" / "old-package" + stale_dir.mkdir(parents=True) + (stale_dir / "apm.yml").write_text("name: old-package\nversion: 0.1.0") + + orphaned = _check_orphaned_packages() + assert "org/my-package" not in orphaned + assert "org/old-package" in orphaned + # --------------------------------------------------------------------------- # _check_and_notify_updates From 9988400ce6afe1369997e00c0162939ba9c63826 Mon Sep 17 00:00:00 2001 From: Travis Illig Date: Wed, 29 Apr 2026 11:18:52 -0700 Subject: [PATCH 03/22] chore: add issue number to changelog, remove unused import Co-Authored-By: Claude Opus 4.6 --- CHANGELOG.md | 2 +- tests/unit/test_command_helpers.py | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d1db2814..9cc9c42a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,7 +37,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- `apm compile`, `apm prune`, and `apm deps list` no longer falsely flag parent directories of subdirectory virtual packages as orphaned. When a dependency uses dict-form (`git:` + `path:`) pointing at a nested path (e.g., `.apm/skills/skill-name`), the intermediate `owner/repo/` directory is now recognized as an ancestor of the expected install path rather than an orphaned package. +- `apm compile`, `apm prune`, and `apm deps list` no longer falsely flag parent directories of subdirectory virtual packages as orphaned. When a dependency uses dict-form (`git:` + `path:`) pointing at a nested path (e.g., `.apm/skills/skill-name`), the intermediate `owner/repo/` directory is now recognized as an ancestor of the expected install path rather than an orphaned package. (#1050) - `apm install` and `apm compile` no longer exit 0 with success messages when `target:` in `apm.yml` is a CSV string -- the value now parses identically to the same input on `--target`, and zero-target resolution surfaces a warning instead of a silent no-op. (#820) - Remove redundant `seen` set from `_scan_patterns()` discovery walk (#918) - `apm pack` (marketplace producer) now respects `GITHUB_HOST` for GitHub Enterprise repos -- ref resolution, token lookup, and metadata fetch all use the configured host instead of hardcoded `github.com`. `git ls-remote` is authenticated so private repos work without separate credential setup. (#1008) diff --git a/tests/unit/test_command_helpers.py b/tests/unit/test_command_helpers.py index 3dac30ac8..a2e24d219 100644 --- a/tests/unit/test_command_helpers.py +++ b/tests/unit/test_command_helpers.py @@ -15,7 +15,6 @@ from apm_cli.commands._helpers import ( _atomic_write, - _build_expected_install_paths, _check_and_notify_updates, _check_orphaned_packages, _get_default_script, From ed227155135552876683d5a5c4415ee780e04f66 Mon Sep 17 00:00:00 2001 From: Travis Illig Date: Wed, 29 Apr 2026 11:20:57 -0700 Subject: [PATCH 04/22] test(deps): cover subdirectory ancestor fix in deps list path Exercises _resolve_scope_deps to ensure owner/repo is not flagged as orphaned when the declared dependency points at a nested subdirectory package beneath it. Co-Authored-By: Claude Opus 4.6 --- tests/unit/test_deps_list_tree_info.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests/unit/test_deps_list_tree_info.py b/tests/unit/test_deps_list_tree_info.py index d48e127b9..59a620fa8 100644 --- a/tests/unit/test_deps_list_tree_info.py +++ b/tests/unit/test_deps_list_tree_info.py @@ -280,6 +280,31 @@ def test_list_insecure_reports_clean_when_no_http_locked_deps(self): assert "No insecure APM dependencies installed" in result.output + def test_list_subdirectory_parent_not_orphaned(self): + """Parent dir of a subdirectory virtual package is not flagged orphaned.""" + with self._chdir_tmp() as tmp: + # Declare a dict-form dependency with path: pointing into .apm/skills/ + (tmp / "apm.yml").write_text( + "name: test-project\n" + "version: 1.0.0\n" + "dependencies:\n" + " apm:\n" + " - git: github.example.com/owner/repo\n" + " path: .apm/skills/my-skill\n" + ) + + # Simulate installed layout: owner/repo/.apm/skills/my-skill/SKILL.md + skill_dir = tmp / "apm_modules" / "owner" / "repo" / ".apm" / "skills" / "my-skill" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text("# Skill") + + with patch("apm_cli.core.scope.get_apm_dir", return_value=tmp), _force_rich_fallback(): + result = self.runner.invoke(cli, ["deps", "list"]) + + assert result.exit_code == 0 + assert "orphan" not in result.output.lower() + + class TestDepsTreeCommand(_DepsCmdBase): """Tests for apm deps tree.""" From 626ff65957bd4c1c1c8346f6cf7641aaad4f1d4a Mon Sep 17 00:00:00 2001 From: Travis Illig Date: Wed, 29 Apr 2026 11:25:10 -0700 Subject: [PATCH 05/22] style(tests): add encoding="utf-8" to all write_text calls Co-Authored-By: Claude Opus 4.6 --- tests/unit/test_command_helpers.py | 14 +++++++------- tests/unit/test_deps_list_tree_info.py | 5 +++-- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/tests/unit/test_command_helpers.py b/tests/unit/test_command_helpers.py index a2e24d219..6d3d487b9 100644 --- a/tests/unit/test_command_helpers.py +++ b/tests/unit/test_command_helpers.py @@ -288,7 +288,7 @@ def test_parent_of_subdirectory_package_not_orphaned(self, tmp_path, monkeypatch apm_modules = tmp_path / "apm_modules" skill_dir = apm_modules / "owner" / "repo" / ".apm" / "skills" / "my-skill" skill_dir.mkdir(parents=True) - (skill_dir / "SKILL.md").write_text("# Skill") + (skill_dir / "SKILL.md").write_text("# Skill", encoding="utf-8") orphaned = _check_orphaned_packages() assert orphaned == [], ( @@ -314,11 +314,11 @@ def test_actual_orphan_still_detected(self, tmp_path, monkeypatch): apm_modules = tmp_path / "apm_modules" skill_dir = apm_modules / "owner" / "repo" / ".apm" / "skills" / "my-skill" skill_dir.mkdir(parents=True) - (skill_dir / "SKILL.md").write_text("# Skill") + (skill_dir / "SKILL.md").write_text("# Skill", encoding="utf-8") orphan_dir = apm_modules / "other" / "stale-pkg" orphan_dir.mkdir(parents=True) - (orphan_dir / "apm.yml").write_text("name: stale-pkg") + (orphan_dir / "apm.yml").write_text("name: stale-pkg", encoding="utf-8") orphaned = _check_orphaned_packages() assert "other/stale-pkg" in orphaned @@ -341,12 +341,12 @@ def test_whole_repo_dependency_not_orphaned(self, tmp_path, monkeypatch): apm_modules = tmp_path / "apm_modules" pkg_dir = apm_modules / "org" / "my-package" pkg_dir.mkdir(parents=True) - (pkg_dir / "apm.yml").write_text("name: my-package\nversion: 1.0.0") + (pkg_dir / "apm.yml").write_text("name: my-package\nversion: 1.0.0", encoding="utf-8") apm_dir = pkg_dir / ".apm" apm_dir.mkdir() instr_dir = apm_dir / "instructions" instr_dir.mkdir() - (instr_dir / "example.instructions.md").write_text("# Instructions") + (instr_dir / "example.instructions.md").write_text("# Instructions", encoding="utf-8") orphaned = _check_orphaned_packages() assert orphaned == [], ( @@ -371,12 +371,12 @@ def test_whole_repo_with_unrelated_orphan(self, tmp_path, monkeypatch): # Declared package pkg_dir = apm_modules / "org" / "my-package" pkg_dir.mkdir(parents=True) - (pkg_dir / "apm.yml").write_text("name: my-package\nversion: 1.0.0") + (pkg_dir / "apm.yml").write_text("name: my-package\nversion: 1.0.0", encoding="utf-8") # Stale package not in apm.yml stale_dir = apm_modules / "org" / "old-package" stale_dir.mkdir(parents=True) - (stale_dir / "apm.yml").write_text("name: old-package\nversion: 0.1.0") + (stale_dir / "apm.yml").write_text("name: old-package\nversion: 0.1.0", encoding="utf-8") orphaned = _check_orphaned_packages() assert "org/my-package" not in orphaned diff --git a/tests/unit/test_deps_list_tree_info.py b/tests/unit/test_deps_list_tree_info.py index 59a620fa8..207ba3e3c 100644 --- a/tests/unit/test_deps_list_tree_info.py +++ b/tests/unit/test_deps_list_tree_info.py @@ -290,13 +290,14 @@ def test_list_subdirectory_parent_not_orphaned(self): "dependencies:\n" " apm:\n" " - git: github.example.com/owner/repo\n" - " path: .apm/skills/my-skill\n" + " path: .apm/skills/my-skill\n", + encoding="utf-8", ) # Simulate installed layout: owner/repo/.apm/skills/my-skill/SKILL.md skill_dir = tmp / "apm_modules" / "owner" / "repo" / ".apm" / "skills" / "my-skill" skill_dir.mkdir(parents=True) - (skill_dir / "SKILL.md").write_text("# Skill") + (skill_dir / "SKILL.md").write_text("# Skill", encoding="utf-8") with patch("apm_cli.core.scope.get_apm_dir", return_value=tmp), _force_rich_fallback(): result = self.runner.invoke(cli, ["deps", "list"]) From c215926989d6a47886f8a9908b15a6bd5d24a133 Mon Sep 17 00:00:00 2001 From: Travis Illig Date: Wed, 29 Apr 2026 11:32:53 -0700 Subject: [PATCH 06/22] Move fix to bottom of list. --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9cc9c42a8..965a3ce94 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,13 +37,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- `apm compile`, `apm prune`, and `apm deps list` no longer falsely flag parent directories of subdirectory virtual packages as orphaned. When a dependency uses dict-form (`git:` + `path:`) pointing at a nested path (e.g., `.apm/skills/skill-name`), the intermediate `owner/repo/` directory is now recognized as an ancestor of the expected install path rather than an orphaned package. (#1050) - `apm install` and `apm compile` no longer exit 0 with success messages when `target:` in `apm.yml` is a CSV string -- the value now parses identically to the same input on `--target`, and zero-target resolution surfaces a warning instead of a silent no-op. (#820) - Remove redundant `seen` set from `_scan_patterns()` discovery walk (#918) - `apm pack` (marketplace producer) now respects `GITHUB_HOST` for GitHub Enterprise repos -- ref resolution, token lookup, and metadata fetch all use the configured host instead of hardcoded `github.com`. `git ls-remote` is authenticated so private repos work without separate credential setup. (#1008) - `apm pack` (marketplace producer) now accepts multiple Git URL forms (GitHub, GHES, GitLab, Bitbucket, ADO, SSH) for `type: url` parsing via `DependencyReference.parse()`. Host resolution is still driven by `GITHUB_HOST`, so non-`github.com` hosts require `GITHUB_HOST` to be set accordingly. (#1008) - **ADO Entra ID auth path no longer silently fails.** Bearer tokens from `az account get-access-token` are now correctly plumbed through validation (auth scheme, git env). Auth failures raise a typed `AuthenticationError` with an actionable 4-case diagnostic instead of the ambiguous "not accessible or doesn't exist" message. `apm install --update` runs a pre-flight auth check before modifying any files -- on failure it aborts with "No files were modified". (#1015) - Correct targeting of compiled artifacts so GEMINI.md is only created if requested (#1019) +- `apm compile`, `apm prune`, and `apm deps list` no longer falsely flag parent directories of subdirectory virtual packages as orphaned. When a dependency uses dict-form (`git:` + `path:`) pointing at a nested path (e.g., `.apm/skills/skill-name`), the intermediate `owner/repo/` directory is now recognized as an ancestor of the expected install path rather than an orphaned package. (#1050) ## [0.10.0] - 2026-04-27 From 2ab7422ad4818ca435cb5526c90a17ac5f9d7323 Mon Sep 17 00:00:00 2001 From: Travis Illig Date: Wed, 29 Apr 2026 12:58:32 -0700 Subject: [PATCH 07/22] Address Copilot review items. --- src/apm_cli/commands/_helpers.py | 23 ++++++++++++---- src/apm_cli/commands/deps/cli.py | 11 ++++---- src/apm_cli/commands/prune.py | 9 +++---- tests/unit/test_command_helpers.py | 36 ++++++++++++++++++++++++++ tests/unit/test_deps_list_tree_info.py | 2 +- 5 files changed, 63 insertions(+), 18 deletions(-) diff --git a/src/apm_cli/commands/_helpers.py b/src/apm_cli/commands/_helpers.py index 7f44b4557..fda752231 100644 --- a/src/apm_cli/commands/_helpers.py +++ b/src/apm_cli/commands/_helpers.py @@ -156,6 +156,22 @@ def _build_expected_install_paths(declared_deps, lockfile, apm_modules_dir: Path return expected +def _expand_with_ancestors(paths) -> set: + """Expand a set of paths to include all ancestor prefixes. + + Given {"owner/repo/.apm/skills/my-skill"}, returns a set containing both + the original path and "owner/repo" (all intermediate path prefixes with + 2+ segments). This allows O(1) membership checks when determining whether + a scanned directory is an ancestor of an expected package path. + """ + expanded = builtins.set(paths) + for p in paths: + parts = p.split("/") + for i in range(2, len(parts)): + expanded.add("/".join(parts[:i])) + return expanded + + def _scan_installed_packages(apm_modules_dir: Path) -> list: """Scan *apm_modules_dir* for installed package paths. @@ -209,11 +225,8 @@ def _check_orphaned_packages(): return [] installed = _scan_installed_packages(apm_modules_dir) - return [ - p for p in installed - if p not in expected - and not any(e.startswith(p + "/") for e in expected) - ] + expected_with_ancestors = _expand_with_ancestors(expected) + return [p for p in installed if p not in expected_with_ancestors] except Exception: return [] diff --git a/src/apm_cli/commands/deps/cli.py b/src/apm_cli/commands/deps/cli.py index 9de339974..a27e3568e 100644 --- a/src/apm_cli/commands/deps/cli.py +++ b/src/apm_cli/commands/deps/cli.py @@ -21,6 +21,7 @@ _get_package_display_info, _get_detailed_package_info, ) +from .._helpers import _expand_with_ancestors # --------------------------------------------------------------------------- @@ -143,6 +144,9 @@ def _resolve_scope_deps(apm_dir, logger, insecure_only=False): except Exception: pass # Continue without lockfile if it can't be read + # Precompute expected paths + ancestors for O(1) orphan checks + declared_with_ancestors = _expand_with_ancestors(declared_sources.keys()) + # Scan for installed packages in org-namespaced structure # Walks the tree to find directories containing apm.yml or SKILL.md, # handling GitHub (2-level), ADO (3-level), and subdirectory (4+ level) packages. @@ -176,12 +180,7 @@ def _resolve_scope_deps(apm_dir, logger, insecure_only=False): version = package.version or 'unknown' primitives = _count_primitives(candidate) - is_orphaned = ( - org_repo_name not in declared_sources - and not any( - k.startswith(org_repo_name + "/") for k in declared_sources - ) - ) + is_orphaned = org_repo_name not in declared_with_ancestors if is_orphaned: orphaned_packages.append(org_repo_name) diff --git a/src/apm_cli/commands/prune.py b/src/apm_cli/commands/prune.py index f67315066..4b20d2803 100644 --- a/src/apm_cli/commands/prune.py +++ b/src/apm_cli/commands/prune.py @@ -9,7 +9,7 @@ from ..constants import APM_LOCK_FILENAME, APM_MODULES_DIR, APM_YML_FILENAME from ..core.command_logger import CommandLogger from ..utils.path_security import PathTraversalError, safe_rmtree -from ._helpers import _build_expected_install_paths, _scan_installed_packages +from ._helpers import _build_expected_install_paths, _expand_with_ancestors, _scan_installed_packages # APM Dependencies from ..deps.lockfile import LockFile, get_lockfile_path @@ -57,11 +57,8 @@ def prune(ctx, dry_run): sys.exit(1) installed_packages = _scan_installed_packages(apm_modules_dir) - orphaned_packages = [ - p for p in installed_packages - if p not in expected_installed - and not any(e.startswith(p + "/") for e in expected_installed) - ] + expected_with_ancestors = _expand_with_ancestors(expected_installed) + orphaned_packages = [p for p in installed_packages if p not in expected_with_ancestors] if not orphaned_packages: logger.success("No orphaned packages found. apm_modules/ is clean.", symbol="check") diff --git a/tests/unit/test_command_helpers.py b/tests/unit/test_command_helpers.py index 6d3d487b9..58f087928 100644 --- a/tests/unit/test_command_helpers.py +++ b/tests/unit/test_command_helpers.py @@ -17,6 +17,7 @@ _atomic_write, _check_and_notify_updates, _check_orphaned_packages, + _expand_with_ancestors, _get_default_script, _list_available_scripts, _load_apm_config, @@ -253,6 +254,41 @@ def test_returns_empty_for_empty_dir(self, tmp_path): assert result == [] +# --------------------------------------------------------------------------- +# _expand_with_ancestors +# --------------------------------------------------------------------------- + + +class TestExpandWithAncestors: + """Tests for _expand_with_ancestors.""" + + def test_adds_intermediate_ancestors(self): + """Subdirectory path produces 2-segment ancestor prefix.""" + paths = {"owner/repo/.apm/skills/my-skill"} + result = _expand_with_ancestors(paths) + assert "owner/repo" in result + assert "owner/repo/.apm" in result + assert "owner/repo/.apm/skills" in result + assert "owner/repo/.apm/skills/my-skill" in result + + def test_two_segment_path_unchanged(self): + """A 2-segment path has no intermediate ancestors to add.""" + paths = {"owner/repo"} + result = _expand_with_ancestors(paths) + assert result == {"owner/repo"} + + def test_empty_input(self): + """Empty set returns empty set.""" + assert _expand_with_ancestors(set()) == set() + + def test_no_false_prefix_overlap(self): + """owner/repo-extra does not collide with owner/repo.""" + paths = {"owner/repo-extra/.apm/skills/x"} + result = _expand_with_ancestors(paths) + assert "owner/repo-extra" in result + assert "owner/repo" not in result + + # --------------------------------------------------------------------------- # _check_orphaned_packages — subdirectory ancestor false-positive fix # --------------------------------------------------------------------------- diff --git a/tests/unit/test_deps_list_tree_info.py b/tests/unit/test_deps_list_tree_info.py index 207ba3e3c..f3769b80a 100644 --- a/tests/unit/test_deps_list_tree_info.py +++ b/tests/unit/test_deps_list_tree_info.py @@ -303,7 +303,7 @@ def test_list_subdirectory_parent_not_orphaned(self): result = self.runner.invoke(cli, ["deps", "list"]) assert result.exit_code == 0 - assert "orphan" not in result.output.lower() + assert "orphaned package(s) found" not in result.output class TestDepsTreeCommand(_DepsCmdBase): From 0275733b46053cd425f173771e4d3f23d0c3560b Mon Sep 17 00:00:00 2001 From: Travis Illig Date: Wed, 29 Apr 2026 13:03:15 -0700 Subject: [PATCH 08/22] Type annotation for _expand_with_ancestors. --- src/apm_cli/commands/_helpers.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/apm_cli/commands/_helpers.py b/src/apm_cli/commands/_helpers.py index fda752231..f279288f5 100644 --- a/src/apm_cli/commands/_helpers.py +++ b/src/apm_cli/commands/_helpers.py @@ -7,6 +7,7 @@ import os import sys import tempfile +from collections.abc import Iterable from pathlib import Path import click @@ -156,7 +157,7 @@ def _build_expected_install_paths(declared_deps, lockfile, apm_modules_dir: Path return expected -def _expand_with_ancestors(paths) -> set: +def _expand_with_ancestors(paths: Iterable[str]) -> set: """Expand a set of paths to include all ancestor prefixes. Given {"owner/repo/.apm/skills/my-skill"}, returns a set containing both From f93d5e14fa5f433b13e42496dfaed3f09f30fd9a Mon Sep 17 00:00:00 2001 From: Travis Illig Date: Wed, 29 Apr 2026 13:03:33 -0700 Subject: [PATCH 09/22] Test for three-segment AzDO paths. --- tests/unit/test_command_helpers.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/unit/test_command_helpers.py b/tests/unit/test_command_helpers.py index 58f087928..11f9312be 100644 --- a/tests/unit/test_command_helpers.py +++ b/tests/unit/test_command_helpers.py @@ -281,6 +281,13 @@ def test_empty_input(self): """Empty set returns empty set.""" assert _expand_with_ancestors(set()) == set() + def test_three_segment_ado_path(self): + """ADO-style org/project/repo produces org/project as ancestor.""" + paths = {"org/project/repo"} + result = _expand_with_ancestors(paths) + assert "org/project/repo" in result + assert "org/project" in result + def test_no_false_prefix_overlap(self): """owner/repo-extra does not collide with owner/repo.""" paths = {"owner/repo-extra/.apm/skills/x"} From 99514afb9ff3737d36e132ad212e24b34c512e96 Mon Sep 17 00:00:00 2001 From: Travis Illig Date: Wed, 29 Apr 2026 17:04:16 -0700 Subject: [PATCH 10/22] Minor formatting fix in changelog. --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aa6b15039..ce2b3c8a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,8 +5,8 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - ## [Unreleased] + ### Added - Slash commands installed from APM packages now surface argument hints in Claude Code -- `apm install` automatically maps prompt `input:` to Claude's `arguments:` front-matter, rewrites `${input:name}` references to `$name`, and auto-generates `argument-hint`. Argument names are validated against an allowlist to prevent YAML injection from third-party packages, and the mapping is reported at install time. (#1039) @@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `apm compile`, `apm prune`, and `apm deps list` no longer falsely flag parent directories of subdirectory virtual packages as orphaned. When a dependency uses dict-form (`git:` + `path:`) pointing at a nested path (e.g., `.apm/skills/skill-name`), the intermediate `owner/repo/` directory is now recognized as an ancestor of the expected install path rather than an orphaned package. (#1050) ## [0.11.0] - 2026-04-29 + ### Added - **`apm pack` is now the single command for marketplace builds** -- with an `apm.yml` `marketplace:` block it emits `.claude-plugin/marketplace.json` directly. New flags: `--offline`, `--include-prerelease`, `--marketplace-output PATH`. (#722) From df4f1aaf7a6a5f4ac8d974fe5043fba1ef199a1b Mon Sep 17 00:00:00 2001 From: Travis Illig Date: Thu, 30 Apr 2026 07:09:38 -0700 Subject: [PATCH 11/22] Fix style issues. --- src/apm_cli/commands/prune.py | 6 +++++- tests/unit/test_command_helpers.py | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/apm_cli/commands/prune.py b/src/apm_cli/commands/prune.py index 79ca75ee4..124be0ae1 100644 --- a/src/apm_cli/commands/prune.py +++ b/src/apm_cli/commands/prune.py @@ -13,7 +13,11 @@ from ..deps.lockfile import LockFile, get_lockfile_path from ..models.apm_package import APMPackage from ..utils.path_security import PathTraversalError, safe_rmtree # noqa: F401 -from ._helpers import _build_expected_install_paths, _expand_with_ancestors, _scan_installed_packages +from ._helpers import ( + _build_expected_install_paths, + _expand_with_ancestors, + _scan_installed_packages, +) @click.command(help="Remove APM packages not listed in apm.yml") diff --git a/tests/unit/test_command_helpers.py b/tests/unit/test_command_helpers.py index 87129fad8..612f92037 100644 --- a/tests/unit/test_command_helpers.py +++ b/tests/unit/test_command_helpers.py @@ -294,7 +294,7 @@ def test_no_false_prefix_overlap(self): # --------------------------------------------------------------------------- -# _check_orphaned_packages — subdirectory ancestor false-positive fix +# _check_orphaned_packages -- subdirectory ancestor false-positive fix # --------------------------------------------------------------------------- From 5f1e8801bb8013d538d247ffbf08c0bd428d2515 Mon Sep 17 00:00:00 2001 From: Travis Illig Date: Thu, 30 Apr 2026 07:09:51 -0700 Subject: [PATCH 12/22] Don't iterate over paths twice. --- src/apm_cli/commands/_helpers.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/apm_cli/commands/_helpers.py b/src/apm_cli/commands/_helpers.py index 435711ff4..0a8f8722e 100644 --- a/src/apm_cli/commands/_helpers.py +++ b/src/apm_cli/commands/_helpers.py @@ -166,8 +166,9 @@ def _expand_with_ancestors(paths: Iterable[str]) -> set: 2+ segments). This allows O(1) membership checks when determining whether a scanned directory is an ancestor of an expected package path. """ - expanded = builtins.set(paths) - for p in paths: + materialized = builtins.list(paths) + expanded = builtins.set(materialized) + for p in materialized: parts = p.split("/") for i in range(2, len(parts)): expanded.add("/".join(parts[:i])) From 2cd125da59d7e46e024303a84a0d847e4f9c4d5e Mon Sep 17 00:00:00 2001 From: Travis Illig Date: Thu, 30 Apr 2026 07:10:56 -0700 Subject: [PATCH 13/22] Improve mock when testing for orphans. --- tests/unit/test_deps_list_tree_info.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/unit/test_deps_list_tree_info.py b/tests/unit/test_deps_list_tree_info.py index d08947504..655ca0f7f 100644 --- a/tests/unit/test_deps_list_tree_info.py +++ b/tests/unit/test_deps_list_tree_info.py @@ -294,8 +294,12 @@ def test_list_subdirectory_parent_not_orphaned(self): encoding="utf-8", ) - # Simulate installed layout: owner/repo/.apm/skills/my-skill/SKILL.md - skill_dir = tmp / "apm_modules" / "owner" / "repo" / ".apm" / "skills" / "my-skill" + # Simulate installed layout: owner/repo/ with apm.yml (so deps list + # scans it as a package) plus the nested skill content. + repo_dir = tmp / "apm_modules" / "owner" / "repo" + repo_dir.mkdir(parents=True) + (repo_dir / "apm.yml").write_text("name: repo\nversion: 1.0.0\n", encoding="utf-8") + skill_dir = repo_dir / ".apm" / "skills" / "my-skill" skill_dir.mkdir(parents=True) (skill_dir / "SKILL.md").write_text("# Skill", encoding="utf-8") From cf54316233c4a45fdd9fbeb39da427bb72cca48f Mon Sep 17 00:00:00 2001 From: Travis Illig Date: Thu, 30 Apr 2026 07:15:25 -0700 Subject: [PATCH 14/22] Update formatting based on contribution guidelines. --- src/apm_cli/commands/deps/cli.py | 2 +- tests/unit/test_command_helpers.py | 8 ++------ tests/unit/test_deps_list_tree_info.py | 1 - 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/src/apm_cli/commands/deps/cli.py b/src/apm_cli/commands/deps/cli.py index 1ee4c00d9..30696644a 100644 --- a/src/apm_cli/commands/deps/cli.py +++ b/src/apm_cli/commands/deps/cli.py @@ -12,6 +12,7 @@ from ...core.command_logger import CommandLogger from ...core.target_detection import TargetParamType from ...models.apm_package import APMPackage, ValidationResult, validate_apm_package # noqa: F401 +from .._helpers import _expand_with_ancestors from ._utils import ( _count_package_files, # noqa: F401 _count_primitives, @@ -21,7 +22,6 @@ _get_package_display_info, _is_nested_under_package, ) -from .._helpers import _expand_with_ancestors # --------------------------------------------------------------------------- # Shared helpers diff --git a/tests/unit/test_command_helpers.py b/tests/unit/test_command_helpers.py index 612f92037..dd1db1dac 100644 --- a/tests/unit/test_command_helpers.py +++ b/tests/unit/test_command_helpers.py @@ -331,9 +331,7 @@ def test_parent_of_subdirectory_package_not_orphaned(self, tmp_path, monkeypatch (skill_dir / "SKILL.md").write_text("# Skill", encoding="utf-8") orphaned = _check_orphaned_packages() - assert orphaned == [], ( - f"Parent dir should not be orphaned; got: {orphaned}" - ) + assert orphaned == [], f"Parent dir should not be orphaned; got: {orphaned}" def test_actual_orphan_still_detected(self, tmp_path, monkeypatch): """A truly orphaned package is still reported even with the ancestor fix.""" @@ -389,9 +387,7 @@ def test_whole_repo_dependency_not_orphaned(self, tmp_path, monkeypatch): (instr_dir / "example.instructions.md").write_text("# Instructions", encoding="utf-8") orphaned = _check_orphaned_packages() - assert orphaned == [], ( - f"Whole-repo package should not be orphaned; got: {orphaned}" - ) + assert orphaned == [], f"Whole-repo package should not be orphaned; got: {orphaned}" def test_whole_repo_with_unrelated_orphan(self, tmp_path, monkeypatch): """Whole-repo dep is fine; an unrelated stale package IS orphaned.""" diff --git a/tests/unit/test_deps_list_tree_info.py b/tests/unit/test_deps_list_tree_info.py index 655ca0f7f..6fcac091b 100644 --- a/tests/unit/test_deps_list_tree_info.py +++ b/tests/unit/test_deps_list_tree_info.py @@ -279,7 +279,6 @@ def test_list_insecure_reports_clean_when_no_http_locked_deps(self): assert result.exit_code == 0 assert "No insecure APM dependencies installed" in result.output - def test_list_subdirectory_parent_not_orphaned(self): """Parent dir of a subdirectory virtual package is not flagged orphaned.""" with self._chdir_tmp() as tmp: From 47bd2c98524fcf0717899b6ba3a3e636e97b637f Mon Sep 17 00:00:00 2001 From: Copilot Date: Thu, 30 Apr 2026 17:58:58 +0200 Subject: [PATCH 15/22] test: tighten orphan-warning assertion (#1052) Match the specific 'orphaned package(s) found' header rather than the generic 'orphan' substring, per Copilot review nit. This avoids a future false-positive match if output ever contains 'orphan' in a non-error context. Other Copilot comments on the PR were resolved earlier (centralized ancestor expansion via _expand_with_ancestors, materialized iterables, import wrapping, ASCII header). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/unit/test_deps_list_tree_info.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/unit/test_deps_list_tree_info.py b/tests/unit/test_deps_list_tree_info.py index 6fcac091b..9e2ecfab3 100644 --- a/tests/unit/test_deps_list_tree_info.py +++ b/tests/unit/test_deps_list_tree_info.py @@ -169,7 +169,9 @@ def test_list_shows_orphaned_warning(self): with patch("apm_cli.core.scope.get_apm_dir", return_value=tmp), _force_rich_fallback(): result = self.runner.invoke(cli, ["deps", "list"]) assert result.exit_code == 0 - assert "orphaned" in result.output.lower() + # Match the specific orphan-warning header (avoids false-positive + # match against unrelated 'orphan' substrings in future output). + assert "orphaned package(s) found" in result.output def test_list_version_shown(self): """Version from apm.yml should appear in fallback text output.""" From 292959be21bb505adea3957e01b027d6d68585cd Mon Sep 17 00:00:00 2001 From: Travis Illig Date: Thu, 30 Apr 2026 10:31:23 -0700 Subject: [PATCH 16/22] Improved changelog mesage for users. --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c4050b421..8523ba9bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,7 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- `apm compile`, `apm prune`, and `apm deps list` no longer falsely flag parent directories of subdirectory virtual packages as orphaned. When a dependency uses dict-form (`git:` + `path:`) pointing at a nested path (e.g., `.apm/skills/skill-name`), the intermediate `owner/repo/` directory is now recognized as an ancestor of the expected install path rather than an orphaned package. (#1050) +- **Orphan detection improved** -- Orphan detection performed by `apm prune`, `apm deps list`, and `apm compile` no longer incorrectly flags the parent directory of a skill installed from a subdirectory path (e.g., `.apm/skills/skill-name`). The folder is now correctly recognized as part of the install tree. (#1050) ## [0.11.0] - 2026-04-29 From 56a91763e50ad28029164a61b162624e3dbd24ed Mon Sep 17 00:00:00 2001 From: Travis Illig Date: Thu, 30 Apr 2026 10:32:08 -0700 Subject: [PATCH 17/22] Improve docs and add path traversal guard to _expand_with_ancestors. --- src/apm_cli/commands/_helpers.py | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/src/apm_cli/commands/_helpers.py b/src/apm_cli/commands/_helpers.py index 0a8f8722e..3c346f6f0 100644 --- a/src/apm_cli/commands/_helpers.py +++ b/src/apm_cli/commands/_helpers.py @@ -158,18 +158,32 @@ def _build_expected_install_paths(declared_deps, lockfile, apm_modules_dir: Path return expected -def _expand_with_ancestors(paths: Iterable[str]) -> set: - """Expand a set of paths to include all ancestor prefixes. - - Given {"owner/repo/.apm/skills/my-skill"}, returns a set containing both - the original path and "owner/repo" (all intermediate path prefixes with - 2+ segments). This allows O(1) membership checks when determining whether - a scanned directory is an ancestor of an expected package path. +def _expand_with_ancestors(paths: Iterable[str]) -> set[str]: + """Expand a set of expected paths to include ancestor prefixes. + + Given ``{"owner/repo/.apm/skills/my-skill"}``, returns a set containing + the original path plus all intermediate path prefixes with 2+ segments + (e.g., ``"owner/repo"``, ``"owner/repo/.apm"``, ``"owner/repo/.apm/skills"``). + This allows O(1) membership checks when determining whether a scanned + directory is an ancestor of an expected package path. + + Ancestor expansion is unconditional because a subdirectory dependency + (``git: owner/repo, path: .apm/skills/x``) is installed by cloning the + entire repo to ``apm_modules/owner/repo/``. The parent directory is + therefore always a required part of the install -- never a stale leftover. + A package at ``owner/repo`` cannot be "genuinely orphaned" while an active + subdirectory dependency references content inside it. + + Safety invariant: ``get_install_path()`` anchors installs at the 2-segment + repo root (GitHub) or 3-segment root (ADO). Ancestor expansion relies on + this -- if the install strategy changes, this function must be re-evaluated. """ materialized = builtins.list(paths) expanded = builtins.set(materialized) for p in materialized: parts = p.split("/") + if ".." in parts: + continue for i in range(2, len(parts)): expanded.add("/".join(parts[:i])) return expanded From c624bc6fef292575db88f32fc7f2c8d36de47910 Mon Sep 17 00:00:00 2001 From: Travis Illig Date: Thu, 30 Apr 2026 10:33:53 -0700 Subject: [PATCH 18/22] Test for path traversal guard. --- tests/unit/test_command_helpers.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/unit/test_command_helpers.py b/tests/unit/test_command_helpers.py index dd1db1dac..89d955931 100644 --- a/tests/unit/test_command_helpers.py +++ b/tests/unit/test_command_helpers.py @@ -292,6 +292,14 @@ def test_no_false_prefix_overlap(self): assert "owner/repo-extra" in result assert "owner/repo" not in result + def test_skips_path_traversal(self): + """Paths containing '..' are skipped during expansion.""" + paths = {"owner/../etc/passwd"} + result = _expand_with_ancestors(paths) + # Original path is kept (it's in the input set), but no ancestors are generated + assert "owner/../etc/passwd" in result + assert "owner/.." not in result + # --------------------------------------------------------------------------- # _check_orphaned_packages -- subdirectory ancestor false-positive fix From 7be5aae9673834809440bacd7aa64f8e55646831 Mon Sep 17 00:00:00 2001 From: Travis Illig Date: Thu, 30 Apr 2026 10:35:55 -0700 Subject: [PATCH 19/22] Change the orphan scanning to two-pass. --- src/apm_cli/commands/deps/cli.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/apm_cli/commands/deps/cli.py b/src/apm_cli/commands/deps/cli.py index 30696644a..99fca2320 100644 --- a/src/apm_cli/commands/deps/cli.py +++ b/src/apm_cli/commands/deps/cli.py @@ -145,14 +145,11 @@ def _resolve_scope_deps(apm_dir, logger, insecure_only=False): except Exception: pass # Continue without lockfile if it can't be read - # Precompute expected paths + ancestors for O(1) orphan checks - declared_with_ancestors = _expand_with_ancestors(declared_sources.keys()) - # Scan for installed packages in org-namespaced structure # Walks the tree to find directories containing apm.yml or SKILL.md, # handling GitHub (2-level), ADO (3-level), and subdirectory (4+ level) packages. - installed_packages = [] - orphaned_packages = [] + # First pass: collect valid candidate paths for ancestor-aware orphan check. + scanned_candidates = [] for candidate in apm_modules_path.rglob("*"): if not candidate.is_dir() or candidate.name.startswith("."): continue @@ -163,8 +160,6 @@ def _resolve_scope_deps(apm_dir, logger, insecure_only=False): rel_parts = candidate.relative_to(apm_modules_path).parts if len(rel_parts) < 2: continue - org_repo_name = "/".join(rel_parts) - # Skip sub-skills inside .apm/ directories -- they belong to the parent package if ".apm" in rel_parts: continue @@ -177,7 +172,14 @@ def _resolve_scope_deps(apm_dir, logger, insecure_only=False): and _is_nested_under_package(candidate, apm_modules_path) ): continue + scanned_candidates.append((candidate, "/".join(rel_parts), has_apm_yml, has_skill_md)) + # Precompute expected paths + ancestors for O(1) orphan checks + declared_with_ancestors = _expand_with_ancestors(declared_sources.keys()) + + installed_packages = [] + orphaned_packages = [] + for candidate, org_repo_name, has_apm_yml, _has_skill_md in scanned_candidates: try: version = "unknown" if has_apm_yml: From 207a7765a62f8343701313b782ccbbc345603619 Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Thu, 30 Apr 2026 21:10:02 +0200 Subject: [PATCH 20/22] fix(prune): keep real-orphan detection when sibling subdir shares owner/repo root + panel nits Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 2 +- src/apm_cli/commands/_helpers.py | 66 ++++++++++++++++++++++++------ tests/unit/test_command_helpers.py | 46 +++++++++++++++++++++ 3 files changed, 100 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 51df3f317..4affbac1a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,7 +22,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- **Orphan detection improved** -- Orphan detection performed by `apm prune`, `apm deps list`, and `apm compile` no longer incorrectly flags the parent directory of a skill installed from a subdirectory path (e.g., `.apm/skills/skill-name`). The folder is now correctly recognized as part of the install tree. (#1050) +- **Orphan detection improved** -- `apm prune` no longer asks you to remove a parent directory you cannot remove: when a skill is installed from a subdirectory path (e.g., `.apm/skills/skill-name`), the cloned `owner/repo/` parent is now correctly recognized as part of the install tree instead of being flagged as an orphan. The same fix applies to `apm deps list` and `apm compile`. A genuinely orphaned `owner/repo` package is still flagged even when a sibling subdirectory dep shares the same `owner/repo` root. (#1050) ## [0.11.0] - 2026-04-29 diff --git a/src/apm_cli/commands/_helpers.py b/src/apm_cli/commands/_helpers.py index 3c346f6f0..34c130e1a 100644 --- a/src/apm_cli/commands/_helpers.py +++ b/src/apm_cli/commands/_helpers.py @@ -158,34 +158,64 @@ def _build_expected_install_paths(declared_deps, lockfile, apm_modules_dir: Path return expected -def _expand_with_ancestors(paths: Iterable[str]) -> set[str]: +def _expand_with_ancestors( + paths: Iterable[str], installed: Iterable[str] | None = None +) -> builtins.set[str]: """Expand a set of expected paths to include ancestor prefixes. Given ``{"owner/repo/.apm/skills/my-skill"}``, returns a set containing the original path plus all intermediate path prefixes with 2+ segments - (e.g., ``"owner/repo"``, ``"owner/repo/.apm"``, ``"owner/repo/.apm/skills"``). + (e.g., ``"owner/repo"``, ``"owner/repo/.apm"``, + ``"owner/repo/.apm/skills"``, plus the original + ``"owner/repo/.apm/skills/my-skill"``). This allows O(1) membership checks when determining whether a scanned directory is an ancestor of an expected package path. - Ancestor expansion is unconditional because a subdirectory dependency + Ancestor expansion exists because a subdirectory dependency (``git: owner/repo, path: .apm/skills/x``) is installed by cloning the - entire repo to ``apm_modules/owner/repo/``. The parent directory is - therefore always a required part of the install -- never a stale leftover. - A package at ``owner/repo`` cannot be "genuinely orphaned" while an active - subdirectory dependency references content inside it. - - Safety invariant: ``get_install_path()`` anchors installs at the 2-segment - repo root (GitHub) or 3-segment root (ADO). Ancestor expansion relies on - this -- if the install strategy changes, this function must be re-evaluated. + entire repo to ``apm_modules/owner/repo/``. Intermediate filesystem + directories created by that clone are required parts of the install -- + not stale leftovers. + + Real-orphan safety: when *installed* is supplied, an ancestor that + matches one of the installed paths is NOT added to the expansion + unless that path is also directly declared in *paths*. Callers should + pass only the subset of installed paths that look like *real + standalone packages* (i.e., directories that ship their own + ``apm.yml``) -- not filesystem intermediaries (which typically have + only a ``.apm/`` subtree from a cloned subdir dep). This preserves + orphan detection for the case where a user has a genuinely orphaned + ``owner/repo`` package on disk alongside a declared sibling + subdirectory dep (``owner/repo/.apm/skills/foo``): only filesystem + intermediaries are suppressed, never real installed packages. + + Safety invariant: ``get_install_path()`` anchors installs at the + 2-segment repo root (GitHub) or 3-segment root (ADO). Ancestor + expansion stops at len(parts) and starts at 2, so for ADO 3-segment + installs only ``org/project`` is added as an ancestor (which is never + a real installed package). If the install strategy changes, this + function must be re-evaluated. + + Traversal guard: any input path containing a ``..`` segment is kept + in the result as-is (membership check) but produces no ancestors. """ materialized = builtins.list(paths) + materialized_set = builtins.set(materialized) expanded = builtins.set(materialized) + installed_set = builtins.set(installed) if installed is not None else builtins.set() for p in materialized: parts = p.split("/") if ".." in parts: continue for i in range(2, len(parts)): - expanded.add("/".join(parts[:i])) + ancestor = "/".join(parts[:i]) + # Do not mask a real installed package via ancestor expansion; + # only filesystem intermediaries should be added. A real + # installed package that is also directly declared remains in + # expanded via materialized_set. + if ancestor in installed_set and ancestor not in materialized_set: + continue + expanded.add(ancestor) return expanded @@ -242,7 +272,17 @@ def _check_orphaned_packages(): return [] installed = _scan_installed_packages(apm_modules_dir) - expected_with_ancestors = _expand_with_ancestors(expected) + # Only paths with their own apm.yml are treated as "real installed + # packages" for ancestor-suppression purposes. A directory with + # only a .apm/ marker is more likely a filesystem intermediary + # produced by cloning a subdirectory dep (the cloned repo root + # contains the .apm/ subtree the dep points into) than a + # standalone published package, which always ships an apm.yml. + # See _expand_with_ancestors for the user-safety rationale. + standalone_installed = [ + p for p in installed if (apm_modules_dir / p / APM_YML_FILENAME).exists() + ] + expected_with_ancestors = _expand_with_ancestors(expected, standalone_installed) return [p for p in installed if p not in expected_with_ancestors] except Exception: return [] diff --git a/tests/unit/test_command_helpers.py b/tests/unit/test_command_helpers.py index 89d955931..c07c7a1c9 100644 --- a/tests/unit/test_command_helpers.py +++ b/tests/unit/test_command_helpers.py @@ -426,6 +426,52 @@ def test_whole_repo_with_unrelated_orphan(self, tmp_path, monkeypatch): assert "org/my-package" not in orphaned assert "org/old-package" in orphaned + def test_real_orphan_at_owner_repo_with_sibling_subdir_dep(self, tmp_path, monkeypatch): + """Regression: a real installed ``owner/repo`` package on disk MUST + still be flagged as orphaned even when a sibling subdirectory dep + ``owner/repo/.apm/skills/foo`` is declared in apm.yml. + + Previously, ancestor expansion blindly added ``owner/repo`` to the + expected set whenever a subdir dep referenced it, silently + suppressing detection of a genuinely orphaned standalone package + that shared the same ``owner/repo`` filesystem root. ``apm prune`` + is a safety command -- it must NEVER silently miss a real orphan. + """ + monkeypatch.chdir(tmp_path) + + # Declare ONLY the subdirectory dep. The standalone owner/repo + # package on disk is NOT declared anywhere. + apm_yml = tmp_path / "apm.yml" + apm_yml.write_text( + "name: test\n" + "version: 1.0.0\n" + "dependencies:\n" + " apm:\n" + " - git: github.example.com/owner/repo\n" + " path: .apm/skills/foo\n", + encoding="utf-8", + ) + + apm_modules = tmp_path / "apm_modules" + # Real installed standalone package at owner/repo (with apm.yml AND + # .apm marker). This is a genuine orphan -- nothing in apm.yml + # declares the whole repo as a dep. + pkg_dir = apm_modules / "owner" / "repo" + pkg_dir.mkdir(parents=True) + (pkg_dir / "apm.yml").write_text("name: repo\nversion: 1.0.0", encoding="utf-8") + # Subdirectory dep content (legitimately installed) shares the + # same ``owner/repo`` root. + skill_dir = pkg_dir / ".apm" / "skills" / "foo" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text("# Skill", encoding="utf-8") + + orphaned = _check_orphaned_packages() + assert "owner/repo" in orphaned, ( + "Real orphan at owner/repo must be flagged even when a " + "sibling subdirectory dep shares the same root; got: " + f"{orphaned}" + ) + # --------------------------------------------------------------------------- # _check_and_notify_updates From f8868ee1328318796004a00d894cf1b56f5c3f95 Mon Sep 17 00:00:00 2001 From: Copilot Date: Thu, 30 Apr 2026 22:19:37 +0200 Subject: [PATCH 21/22] fix(orphan-detect): close real-orphan deletion gap + harden ancestor expansion Fold panel round-2 findings (microsoft/apm#1052): - prune.py: pass standalone_installed (lockfile-membership + apm.yml fallback) to _expand_with_ancestors so the destructive command behaves identically to its advisory display path. Closes the real-orphan deletion false-negative when a sibling subdir dep shares the same owner/repo install root. - deps/cli.py: thread the same standalone_installed guard through _resolve_scope_deps; route both orphan-warning blocks (Rich and fallback) through CommandLogger.warning() instead of raw console.print/click.echo. Removes the latent Rich MarkupError risk from the '[!]' literal and unifies output behaviour with prune.py. - _helpers.py: normalise backslashes before the '..' split guard in _expand_with_ancestors (closes the backslash-traversal bypass); cap ancestor emission at depth 3 (ADO install-root depth) to bound the orphan-suppression surface; replace the apm.yml-only heuristic with a combined lockfile-membership + apm.yml signal via new _standalone_installed_packages helper (tamper-evident determination); sort the orphan list for deterministic output. - CHANGELOG: rewrite the Fixed entry in user-voice per CEO-adopted growth-hacker suggestion; surface the multi-skill / monorepo positioning angle and the no-migration-needed reassurance. - Tests: add regression coverage for (a) prune CLI deleting a real owner/repo orphan when a sibling subdir dep is declared, (b) backslash-traversal guard, (c) installed= guard semantic, (d) depth-cap; update the deps-list parent-not-orphaned test to use the simpler intermediary-only fixture per panel nit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 2 +- src/apm_cli/commands/_helpers.py | 96 ++++++++++++++++++++------ src/apm_cli/commands/deps/cli.py | 53 +++++++++----- src/apm_cli/commands/prune.py | 17 ++++- tests/unit/test_command_helpers.py | 55 ++++++++++++++- tests/unit/test_deps_list_tree_info.py | 21 ++++-- tests/unit/test_prune_command.py | 73 ++++++++++++++++++++ 7 files changed, 270 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4affbac1a..e2a64cc1a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,7 +22,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- **Orphan detection improved** -- `apm prune` no longer asks you to remove a parent directory you cannot remove: when a skill is installed from a subdirectory path (e.g., `.apm/skills/skill-name`), the cloned `owner/repo/` parent is now correctly recognized as part of the install tree instead of being flagged as an orphan. The same fix applies to `apm deps list` and `apm compile`. A genuinely orphaned `owner/repo` package is still flagged even when a sibling subdirectory dep shares the same `owner/repo` root. (#1050) +- **`apm prune` no longer flags directories it put there itself** -- skills installed from a subdirectory path (e.g., `owner/repo/.apm/skills/skill-name`) no longer cause the parent `owner/repo/` clone to appear as an orphan. Fixes spurious removal prompts in multi-skill and monorepo-style setups. The same fix applies to `apm deps list` and `apm compile`. A genuinely orphaned `owner/repo` package is still flagged even when a sibling subdirectory dep shares the same `owner/repo` root. No changes to `apm.yml` or the lockfile are required to benefit from this fix. (#1050) ## [0.11.0] - 2026-04-29 diff --git a/src/apm_cli/commands/_helpers.py b/src/apm_cli/commands/_helpers.py index 34c130e1a..95a91d0b7 100644 --- a/src/apm_cli/commands/_helpers.py +++ b/src/apm_cli/commands/_helpers.py @@ -189,25 +189,40 @@ def _expand_with_ancestors( subdirectory dep (``owner/repo/.apm/skills/foo``): only filesystem intermediaries are suppressed, never real installed packages. - Safety invariant: ``get_install_path()`` anchors installs at the - 2-segment repo root (GitHub) or 3-segment root (ADO). Ancestor - expansion stops at len(parts) and starts at 2, so for ADO 3-segment - installs only ``org/project`` is added as an ancestor (which is never - a real installed package). If the install strategy changes, this - function must be re-evaluated. - - Traversal guard: any input path containing a ``..`` segment is kept - in the result as-is (membership check) but produces no ancestors. + Security contract -- ancestor depth cap: ``get_install_path()`` + anchors installs at the 2-segment repo root (GitHub) or 3-segment + root (ADO). Anything deeper is a filesystem-intermediary path + (``.apm/``, ``skills/``, ...) that ``_scan_installed_packages`` + skips, so emitting ancestors past depth 3 would only widen the + orphan-suppression surface without serving any real lookup. The + loop is therefore capped at depth 3 (``min(4, len(parts))``), which + bounds the number of paths an attacker-influenced ``apm.yml`` dep + declaration can hide from orphan detection. If the install strategy + ever grows deeper roots, lift this cap and document the new + invariant here. + + Traversal guard: any input path containing a ``..`` segment after + backslash normalisation is kept in the result as-is (membership + check) but produces no ancestors. Backslashes are normalised to + forward slashes before splitting so a path like + ``owner\\..\\evil`` cannot bypass the check by appearing as a + single unsplit token on POSIX hosts. """ materialized = builtins.list(paths) materialized_set = builtins.set(materialized) expanded = builtins.set(materialized) installed_set = builtins.set(installed) if installed is not None else builtins.set() for p in materialized: - parts = p.split("/") + # Normalise backslashes so Windows-style or attacker-crafted + # tokens like "owner\\..\\evil" cannot smuggle a traversal + # segment past the '..' guard below. + normalised = p.replace("\\", "/") + parts = normalised.split("/") if ".." in parts: continue - for i in range(2, len(parts)): + # Cap at depth 3 -- the ADO install-root depth -- to bound the + # ancestor-suppression surface (see security contract above). + for i in range(2, min(4, len(parts))): ancestor = "/".join(parts[:i]) # Do not mask a real installed package via ancestor expansion; # only filesystem intermediaries should be added. A real @@ -242,6 +257,44 @@ def _scan_installed_packages(apm_modules_dir: Path) -> list: return installed +def _standalone_installed_packages( + installed: Iterable[str], apm_modules_dir: Path, lockfile=None +) -> list: + """Filter *installed* to entries that look like real standalone packages. + + Determination order (tamper-evident first): + + 1. Path appears as a dependency key in *lockfile* -- the canonical + record of what APM installed. The lockfile is integrity-checked + and not forgeable by dropping/omitting files in ``apm_modules/``. + 2. Fallback: path has its own ``apm.yml``. Used when the lockfile + is absent (older installs / fresh checkouts) or does not list + the key. A directory with only a ``.apm/`` marker is treated as + a filesystem intermediary, not a standalone package. + + Combining both signals closes the suppression-via-absence gap + (panel finding: forgeable ``apm.yml`` heuristic) while preserving + behaviour for projects that pre-date the lockfile or have not yet + re-installed. + """ + lockfile_keys: builtins.set[str] = builtins.set() + if lockfile is not None: + try: + for dep_key in lockfile.dependencies: + if dep_key: + lockfile_keys.add(dep_key) + except Exception: + lockfile_keys = builtins.set() + standalone: list = [] + for p in installed: + if p in lockfile_keys: + standalone.append(p) + continue + if (apm_modules_dir / p / APM_YML_FILENAME).exists(): + standalone.append(p) + return standalone + + def _check_orphaned_packages(): """Check for packages in apm_modules/ that are not declared in apm.yml or apm.lock. @@ -272,18 +325,19 @@ def _check_orphaned_packages(): return [] installed = _scan_installed_packages(apm_modules_dir) - # Only paths with their own apm.yml are treated as "real installed - # packages" for ancestor-suppression purposes. A directory with - # only a .apm/ marker is more likely a filesystem intermediary - # produced by cloning a subdirectory dep (the cloned repo root - # contains the .apm/ subtree the dep points into) than a - # standalone published package, which always ships an apm.yml. + # Combined lockfile-membership + apm.yml fallback determines + # which installed paths are real standalone packages (and so + # must NOT be masked by ancestor expansion). The lockfile is + # the canonical, tamper-evident record; apm.yml-existence is + # the fallback for projects without a lockfile yet. # See _expand_with_ancestors for the user-safety rationale. - standalone_installed = [ - p for p in installed if (apm_modules_dir / p / APM_YML_FILENAME).exists() - ] + standalone_installed = _standalone_installed_packages( + installed, apm_modules_dir, lockfile=lockfile + ) expected_with_ancestors = _expand_with_ancestors(expected, standalone_installed) - return [p for p in installed if p not in expected_with_ancestors] + # Sort for deterministic, diffable output across runs (rglob + # traversal order is filesystem-dependent). + return sorted(p for p in installed if p not in expected_with_ancestors) except Exception: return [] diff --git a/src/apm_cli/commands/deps/cli.py b/src/apm_cli/commands/deps/cli.py index 99fca2320..9d8998b2c 100644 --- a/src/apm_cli/commands/deps/cli.py +++ b/src/apm_cli/commands/deps/cli.py @@ -12,7 +12,7 @@ from ...core.command_logger import CommandLogger from ...core.target_detection import TargetParamType from ...models.apm_package import APMPackage, ValidationResult, validate_apm_package # noqa: F401 -from .._helpers import _expand_with_ancestors +from .._helpers import _expand_with_ancestors, _standalone_installed_packages from ._utils import ( _count_package_files, # noqa: F401 _count_primitives, @@ -174,8 +174,28 @@ def _resolve_scope_deps(apm_dir, logger, insecure_only=False): continue scanned_candidates.append((candidate, "/".join(rel_parts), has_apm_yml, has_skill_md)) - # Precompute expected paths + ancestors for O(1) orphan checks - declared_with_ancestors = _expand_with_ancestors(declared_sources.keys()) + # Precompute expected paths + ancestors for O(1) orphan checks. + # Mirror prune.py / _check_orphaned_packages: pass the standalone + # installed paths (lockfile-membership + apm.yml fallback) so a + # genuinely orphaned ``owner/repo`` package is not masked when a + # sibling subdirectory dep shares the same install root. + try: + try: + lockfile_path_for_check = get_lockfile_path(apm_dir) + lockfile_for_check = ( + LockFile.read(lockfile_path_for_check) if lockfile_path_for_check.exists() else None + ) + except Exception: + lockfile_for_check = None + scanned_names = [name for _c, name, _h, _s in scanned_candidates] + standalone_installed_for_check = _standalone_installed_packages( + scanned_names, apm_modules_path, lockfile=lockfile_for_check + ) + except Exception: + standalone_installed_for_check = [] + declared_with_ancestors = _expand_with_ancestors( + declared_sources.keys(), standalone_installed_for_check + ) installed_packages = [] orphaned_packages = [] @@ -216,7 +236,7 @@ def _resolve_scope_deps(apm_dir, logger, insecure_only=False): if insecure_only: installed_packages = [pkg for pkg in installed_packages if pkg["is_insecure"]] - return installed_packages, orphaned_packages + return installed_packages, sorted(orphaned_packages) @click.group(help="Manage APM package dependencies") @@ -283,15 +303,15 @@ def _show_scope_deps(scope_label, apm_dir, logger, console, has_rich, insecure_o console.print(table) - # Show orphaned packages warning + # Show orphaned packages warning -- routed through CommandLogger + # so output goes through the central STATUS_SYMBOLS prefix path + # (no raw `[!]` literal that Rich would parse as markup) and so + # behaviour is consistent with prune.py. if orphaned_packages: - console.print( - f"\n[!] {len(orphaned_packages)} orphaned package(s) found (not in apm.yml):", - style="yellow", - ) + logger.warning(f"{len(orphaned_packages)} orphaned package(s) found (not in apm.yml):") for pkg in orphaned_packages: - console.print(f" * {pkg}", style="dim yellow") - console.print("\n Run 'apm prune' to remove orphaned packages", style="cyan") + logger.progress(f" - {pkg}") + logger.progress("Run 'apm prune' to remove orphaned packages") else: # Fallback text table if insecure_only: @@ -329,14 +349,13 @@ def _show_scope_deps(scope_label, apm_dir, logger, console, has_rich, insecure_o f"{name:<30} {version:<10} {source:<12} {prompts:>7} {instructions:>7} {agents:>7} {skills:>7} {hooks:>7}" ) - # Show orphaned packages warning + # Show orphaned packages warning -- route through CommandLogger + # for consistency with the rich branch above and with prune.py. if orphaned_packages: - click.echo( - f"\n[!] {len(orphaned_packages)} orphaned package(s) found (not in apm.yml):" - ) + logger.warning(f"{len(orphaned_packages)} orphaned package(s) found (not in apm.yml):") for pkg in orphaned_packages: - click.echo(f" * {pkg}") - click.echo("\n Run 'apm prune' to remove orphaned packages") + logger.progress(f" - {pkg}") + logger.progress("Run 'apm prune' to remove orphaned packages") @deps.command(name="list", help="List installed APM dependencies") diff --git a/src/apm_cli/commands/prune.py b/src/apm_cli/commands/prune.py index 124be0ae1..86bc7b60b 100644 --- a/src/apm_cli/commands/prune.py +++ b/src/apm_cli/commands/prune.py @@ -17,6 +17,7 @@ _build_expected_install_paths, _expand_with_ancestors, _scan_installed_packages, + _standalone_installed_packages, ) @@ -61,8 +62,20 @@ def prune(ctx, dry_run): sys.exit(1) installed_packages = _scan_installed_packages(apm_modules_dir) - expected_with_ancestors = _expand_with_ancestors(expected_installed) - orphaned_packages = [p for p in installed_packages if p not in expected_with_ancestors] + # Mirror _check_orphaned_packages: filter installed paths to + # real standalone packages (lockfile-membership + apm.yml + # fallback) so ancestor expansion does NOT silently mask a + # genuinely orphaned ``owner/repo`` package when a sibling + # subdirectory dep shares the same install root. + # ``apm prune`` is a destructive command -- it MUST behave + # identically to its advisory display path. + standalone_installed = _standalone_installed_packages( + installed_packages, apm_modules_dir, lockfile=lockfile + ) + expected_with_ancestors = _expand_with_ancestors(expected_installed, standalone_installed) + orphaned_packages = sorted( + p for p in installed_packages if p not in expected_with_ancestors + ) if not orphaned_packages: logger.success("No orphaned packages found. apm_modules/ is clean.", symbol="check") diff --git a/tests/unit/test_command_helpers.py b/tests/unit/test_command_helpers.py index c07c7a1c9..98bfbc816 100644 --- a/tests/unit/test_command_helpers.py +++ b/tests/unit/test_command_helpers.py @@ -260,13 +260,20 @@ class TestExpandWithAncestors: """Tests for _expand_with_ancestors.""" def test_adds_intermediate_ancestors(self): - """Subdirectory path produces 2-segment ancestor prefix.""" + """Subdirectory path produces install-root ancestors only. + + Depth-cap security contract: ancestors past depth 3 are NOT + emitted (see _expand_with_ancestors docstring). For a path + like ``owner/repo/.apm/skills/my-skill`` only the 2-segment + and 3-segment prefixes are added. + """ paths = {"owner/repo/.apm/skills/my-skill"} result = _expand_with_ancestors(paths) assert "owner/repo" in result assert "owner/repo/.apm" in result - assert "owner/repo/.apm/skills" in result assert "owner/repo/.apm/skills/my-skill" in result + # Depth cap: deeper ancestors are intentionally not emitted. + assert "owner/repo/.apm/skills" not in result def test_two_segment_path_unchanged(self): """A 2-segment path has no intermediate ancestors to add.""" @@ -300,6 +307,50 @@ def test_skips_path_traversal(self): assert "owner/../etc/passwd" in result assert "owner/.." not in result + def test_skips_backslash_traversal(self): + """Backslash-encoded traversal cannot bypass the '..' guard. + + Regression for the supply-chain finding: prior implementation + called ``p.split('/')`` directly, so a token like + ``owner\\..\\evil/sub`` parsed as a single segment containing + ``..`` and slipped past the guard. The fix normalises ``\\`` + -> ``/`` before splitting. + """ + paths = {"owner\\..\\evil/sub"} + result = _expand_with_ancestors(paths) + # Original kept (membership semantics), but NO ancestor must + # leak into the expansion. + assert "owner\\..\\evil/sub" in result + assert "owner" not in result + assert "owner/.." not in result + assert "owner\\..\\evil" not in result + + def test_installed_guard_protects_real_orphan(self): + """When ``installed`` lists a real standalone package that is + also an ancestor of an expected subdir dep, the ancestor is + NOT added to the expansion -- so the real package can still + be detected as an orphan. + """ + paths = {"owner/repo/.apm/skills/foo"} + result = _expand_with_ancestors(paths, installed={"owner/repo"}) + assert "owner/repo" not in result, ( + "Real installed package must not be masked by ancestor expansion" + ) + + def test_depth_cap_bounds_ancestor_emission(self): + """Ancestors past depth 3 are not emitted (security cap). + + ``_scan_installed_packages`` skips dotted dirs and doesn't see + intermediates past depth 3, so emitting them would only widen + the orphan-suppression surface. + """ + paths = {"owner/repo/.apm/skills/foo/extra/deeper"} + result = _expand_with_ancestors(paths) + assert "owner/repo" in result + # Cap stops emission at depth 3 (exclusive index 4). + assert "owner/repo/.apm/skills" not in result + assert "owner/repo/.apm/skills/foo" not in result + # --------------------------------------------------------------------------- # _check_orphaned_packages -- subdirectory ancestor false-positive fix diff --git a/tests/unit/test_deps_list_tree_info.py b/tests/unit/test_deps_list_tree_info.py index 9e2ecfab3..d71de4195 100644 --- a/tests/unit/test_deps_list_tree_info.py +++ b/tests/unit/test_deps_list_tree_info.py @@ -282,7 +282,17 @@ def test_list_insecure_reports_clean_when_no_http_locked_deps(self): assert "No insecure APM dependencies installed" in result.output def test_list_subdirectory_parent_not_orphaned(self): - """Parent dir of a subdirectory virtual package is not flagged orphaned.""" + """Parent dir of a subdirectory virtual package is not flagged orphaned. + + Per panel feedback (test fixture nit): the simpler -- and more + accurate -- representation of the subdir-dep bug uses NO + ``apm.yml`` at the ``owner/repo`` parent (only the ``.apm/`` + marker created by the clone). With ``apm.yml`` present at + ``owner/repo`` the directory is a real standalone package and + the new ``standalone_installed`` guard correctly flags it as + orphaned (it is not declared anywhere). Drop the conflated + ``apm.yml`` to test the intended scenario. + """ with self._chdir_tmp() as tmp: # Declare a dict-form dependency with path: pointing into .apm/skills/ (tmp / "apm.yml").write_text( @@ -295,11 +305,11 @@ def test_list_subdirectory_parent_not_orphaned(self): encoding="utf-8", ) - # Simulate installed layout: owner/repo/ with apm.yml (so deps list - # scans it as a package) plus the nested skill content. + # Simulate installed layout: owner/repo/ with NO apm.yml -- + # only the cloned .apm/ subtree. ``owner/repo`` is a pure + # filesystem intermediary, not a standalone package. repo_dir = tmp / "apm_modules" / "owner" / "repo" repo_dir.mkdir(parents=True) - (repo_dir / "apm.yml").write_text("name: repo\nversion: 1.0.0\n", encoding="utf-8") skill_dir = repo_dir / ".apm" / "skills" / "my-skill" skill_dir.mkdir(parents=True) (skill_dir / "SKILL.md").write_text("# Skill", encoding="utf-8") @@ -308,6 +318,9 @@ def test_list_subdirectory_parent_not_orphaned(self): result = self.runner.invoke(cli, ["deps", "list"]) assert result.exit_code == 0 + # Assertion targets the deps-specific orphan header + # (deps says "orphaned package(s) found", prune says + # "Found N orphaned package(s)"). assert "orphaned package(s) found" not in result.output diff --git a/tests/unit/test_prune_command.py b/tests/unit/test_prune_command.py index f7f868bb2..713d66cd4 100644 --- a/tests/unit/test_prune_command.py +++ b/tests/unit/test_prune_command.py @@ -225,6 +225,79 @@ def test_prune_removes_multiple_orphans(self): assert not dir1.exists() assert not dir2.exists() + def test_prune_removes_real_orphan_with_sibling_subdir_dep(self): + """Regression: the destructive ``apm prune`` command must + delete a genuinely orphaned ``owner/repo`` package even when + a sibling subdirectory dep ``owner/repo/.apm/skills/foo`` is + declared in apm.yml. + + Previously, ``prune.py`` called ``_expand_with_ancestors`` + without the ``standalone_installed`` guard, so ``owner/repo`` + was added to the expected set as an ancestor of the subdir + dep -- silently suppressing deletion of a real orphan and + diverging from the advisory display path. ``apm prune`` is a + safety command; missing a real orphan is a correctness bug. + """ + with self._chdir_tmp() as tmp: + # Declare ONLY the subdirectory dep. The standalone + # owner/repo package is not declared anywhere. + (tmp / "apm.yml").write_text( + "name: test\n" + "version: 1.0.0\n" + "dependencies:\n" + " apm:\n" + " - git: github.example.com/owner/repo\n" + " path: .apm/skills/foo\n" + ) + # Real installed standalone package (apm.yml + .apm marker). + pkg_dir = tmp / "apm_modules" / "owner" / "repo" + pkg_dir.mkdir(parents=True) + (pkg_dir / "apm.yml").write_text("name: repo\nversion: 1.0\n") + # Subdirectory dep content cohabits the same install root. + skill_dir = pkg_dir / ".apm" / "skills" / "foo" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text("# Skill\n") + + result = self.runner.invoke(cli, ["prune"]) + assert result.exit_code == 0, result.output + # Real orphan MUST be deleted -- this is the security + # invariant the panel flagged as a required fix. + assert not (pkg_dir / "apm.yml").exists(), ( + "Real orphan owner/repo (apm.yml) must be removed even " + "when a sibling subdir dep shares the same root" + ) + # Subdir dep content collateral-damages because the whole + # owner/repo tree is the orphan's filesystem footprint; + # the user is expected to re-install. This matches the + # advisory display path in deps/cli.py. + assert not skill_dir.exists() + + def test_prune_dry_run_lists_real_orphan_with_sibling_subdir_dep(self): + """Dry-run path must also surface the real orphan (display + parity with the advisory check). + """ + with self._chdir_tmp() as tmp: + (tmp / "apm.yml").write_text( + "name: test\n" + "version: 1.0.0\n" + "dependencies:\n" + " apm:\n" + " - git: github.example.com/owner/repo\n" + " path: .apm/skills/foo\n" + ) + pkg_dir = tmp / "apm_modules" / "owner" / "repo" + pkg_dir.mkdir(parents=True) + (pkg_dir / "apm.yml").write_text("name: repo\nversion: 1.0\n") + skill_dir = pkg_dir / ".apm" / "skills" / "foo" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text("# Skill\n") + + result = self.runner.invoke(cli, ["prune", "--dry-run"]) + assert result.exit_code == 0, result.output + assert "owner/repo" in result.output + # No deletion occurred. + assert (pkg_dir / "apm.yml").exists() + # ------------------------------------------------------------------ # Parse error in apm.yml # ------------------------------------------------------------------ From 5cda91b088ece951e9b468cd329b53351225bde3 Mon Sep 17 00:00:00 2001 From: Copilot Date: Thu, 30 Apr 2026 23:06:23 +0200 Subject: [PATCH 22/22] Fix panel round 3: security gates + orphan announce parity Persona-gated round 3 fold for #1052. 4 findings: - validate_path_segments replaces ad-hoc .. check in _helpers.py - _standalone_installed_packages no longer swallows lockfile errors - prune.py + deps/cli.py orphan surface unified on logger.warning - builtins.set / builtins.list shims removed (redundant) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/apm_cli/commands/_helpers.py | 52 ++++--- src/apm_cli/commands/deps/cli.py | 8 +- src/apm_cli/commands/prune.py | 6 +- src/apm_cli/core/command_logger.py | 13 ++ tests/unit/test_command_helpers.py | 87 ++++++++++++ tests/unit/test_orphan_announce_parity.py | 160 ++++++++++++++++++++++ 6 files changed, 299 insertions(+), 27 deletions(-) create mode 100644 tests/unit/test_orphan_announce_parity.py diff --git a/src/apm_cli/commands/_helpers.py b/src/apm_cli/commands/_helpers.py index 95a91d0b7..9a441a400 100644 --- a/src/apm_cli/commands/_helpers.py +++ b/src/apm_cli/commands/_helpers.py @@ -24,6 +24,7 @@ from ..update_policy import get_update_hint_message, is_self_update_enabled from ..utils.atomic_io import atomic_write_text as _atomic_write # noqa: F401 from ..utils.console import _rich_echo, _rich_info, _rich_warning +from ..utils.path_security import PathTraversalError, validate_path_segments from ..utils.version_checker import check_for_updates from ..version import get_build_sha, get_version @@ -136,7 +137,7 @@ def _build_expected_install_paths(declared_deps, lockfile, apm_modules_dir: Path (depth > 1 from ``apm.lock``), using ``get_install_path()`` for consistency with how packages are actually installed. """ - expected = builtins.set() + expected = set() for dep in declared_deps: install_path = dep.get_install_path(apm_modules_dir) try: @@ -160,7 +161,7 @@ def _build_expected_install_paths(declared_deps, lockfile, apm_modules_dir: Path def _expand_with_ancestors( paths: Iterable[str], installed: Iterable[str] | None = None -) -> builtins.set[str]: +) -> set[str]: """Expand a set of expected paths to include ancestor prefixes. Given ``{"owner/repo/.apm/skills/my-skill"}``, returns a set containing @@ -201,25 +202,28 @@ def _expand_with_ancestors( ever grows deeper roots, lift this cap and document the new invariant here. - Traversal guard: any input path containing a ``..`` segment after - backslash normalisation is kept in the result as-is (membership - check) but produces no ancestors. Backslashes are normalised to - forward slashes before splitting so a path like - ``owner\\..\\evil`` cannot bypass the check by appearing as a - single unsplit token on POSIX hosts. + Traversal guard: any input path that fails + :func:`apm_cli.utils.path_security.validate_path_segments` (which + rejects both ``.`` and ``..`` segments after backslash + normalisation) is kept in the result as-is (membership check) but + produces no ancestors. Routing through the canonical guard -- + rather than a hand-rolled ``".." in parts`` check -- ensures + single-dot segments (``owner/./repo``) are also caught and keeps + the project's path-validation contract centralised. """ - materialized = builtins.list(paths) - materialized_set = builtins.set(materialized) - expanded = builtins.set(materialized) - installed_set = builtins.set(installed) if installed is not None else builtins.set() + materialized = list(paths) + materialized_set = set(materialized) + expanded = set(materialized) + installed_set = set(installed) if installed is not None else set() for p in materialized: - # Normalise backslashes so Windows-style or attacker-crafted - # tokens like "owner\\..\\evil" cannot smuggle a traversal - # segment past the '..' guard below. + try: + validate_path_segments(p, context="ancestor expansion") + except PathTraversalError: + continue + # Normalise backslashes so Windows-style tokens split into the + # same parts as POSIX inputs for the depth-capped loop below. normalised = p.replace("\\", "/") parts = normalised.split("/") - if ".." in parts: - continue # Cap at depth 3 -- the ADO install-root depth -- to bound the # ancestor-suppression surface (see security contract above). for i in range(2, min(4, len(parts))): @@ -276,15 +280,23 @@ def _standalone_installed_packages( (panel finding: forgeable ``apm.yml`` heuristic) while preserving behaviour for projects that pre-date the lockfile or have not yet re-installed. + + Failure mode: only narrowly-typed shape errors against + ``lockfile.dependencies`` (``AttributeError`` / ``TypeError`` / + ``KeyError``) are absorbed and degrade to the ``apm.yml``-only + fallback. Any other exception (e.g. lockfile parse / I/O failure) + propagates so the outer caller can decide whether to log or fail + closed -- preventing a corrupted or attacker-crafted lockfile from + silently disabling the tamper-evident standalone check. """ - lockfile_keys: builtins.set[str] = builtins.set() + lockfile_keys: set[str] = set() if lockfile is not None: try: for dep_key in lockfile.dependencies: if dep_key: lockfile_keys.add(dep_key) - except Exception: - lockfile_keys = builtins.set() + except (AttributeError, TypeError, KeyError): + lockfile_keys = set() standalone: list = [] for p in installed: if p in lockfile_keys: diff --git a/src/apm_cli/commands/deps/cli.py b/src/apm_cli/commands/deps/cli.py index 9d8998b2c..49a7197b9 100644 --- a/src/apm_cli/commands/deps/cli.py +++ b/src/apm_cli/commands/deps/cli.py @@ -310,8 +310,8 @@ def _show_scope_deps(scope_label, apm_dir, logger, console, has_rich, insecure_o if orphaned_packages: logger.warning(f"{len(orphaned_packages)} orphaned package(s) found (not in apm.yml):") for pkg in orphaned_packages: - logger.progress(f" - {pkg}") - logger.progress("Run 'apm prune' to remove orphaned packages") + logger.warning(f" - {pkg}") + logger.info("Run 'apm prune' to remove orphaned packages") else: # Fallback text table if insecure_only: @@ -354,8 +354,8 @@ def _show_scope_deps(scope_label, apm_dir, logger, console, has_rich, insecure_o if orphaned_packages: logger.warning(f"{len(orphaned_packages)} orphaned package(s) found (not in apm.yml):") for pkg in orphaned_packages: - logger.progress(f" - {pkg}") - logger.progress("Run 'apm prune' to remove orphaned packages") + logger.warning(f" - {pkg}") + logger.info("Run 'apm prune' to remove orphaned packages") @deps.command(name="list", help="List installed APM dependencies") diff --git a/src/apm_cli/commands/prune.py b/src/apm_cli/commands/prune.py index 86bc7b60b..063170cc2 100644 --- a/src/apm_cli/commands/prune.py +++ b/src/apm_cli/commands/prune.py @@ -82,12 +82,12 @@ def prune(ctx, dry_run): return # Show what will be removed - logger.progress(f"Found {len(orphaned_packages)} orphaned package(s):") + logger.warning(f"Found {len(orphaned_packages)} orphaned package(s):") for pkg_name in orphaned_packages: if dry_run: - logger.progress(f" - {pkg_name} (would be removed)") + logger.warning(f" - {pkg_name} (would be removed)") else: - logger.progress(f" - {pkg_name}") + logger.warning(f" - {pkg_name}") if dry_run: logger.success("Dry run complete - no changes made") diff --git a/src/apm_cli/core/command_logger.py b/src/apm_cli/core/command_logger.py index 8c0351e11..506db7683 100644 --- a/src/apm_cli/core/command_logger.py +++ b/src/apm_cli/core/command_logger.py @@ -86,6 +86,19 @@ def progress(self, message: str, symbol: str = "info"): """Log progress during an operation.""" _rich_info(message, symbol=symbol) + def info(self, message: str, symbol: str = "info"): + """Log static advisory / informational context. + + Distinct from :meth:`progress` only at the semantic level: + ``progress`` narrates an in-flight step (may be suppressed in + ``--quiet``/CI), while ``info`` carries persistent advisory + context such as recovery hints that must survive quiet-mode + suppression. Both currently delegate to ``_rich_info``; the + split exists so future quiet-mode policy can drop ``progress`` + without dropping advisory context. + """ + _rich_info(message, symbol=symbol) + def success(self, message: str, symbol: str = "sparkles"): """Log successful completion.""" _rich_success(message, symbol=symbol) diff --git a/tests/unit/test_command_helpers.py b/tests/unit/test_command_helpers.py index 98bfbc816..9a41f988a 100644 --- a/tests/unit/test_command_helpers.py +++ b/tests/unit/test_command_helpers.py @@ -588,3 +588,90 @@ def test_silently_ignores_check_exception(self): ): # Should not raise _check_and_notify_updates() + + +# --------------------------------------------------------------------------- +# Round-3 panel regressions: +# - traversal guard must route through canonical validate_path_segments +# - _standalone_installed_packages must NOT swallow corruption errors +# --------------------------------------------------------------------------- + + +class TestExpandWithAncestorsRoutesThroughCanonicalGuard: + """Round-3 supply-chain finding: ad-hoc ``..`` check must be replaced + by ``apm_cli.utils.path_security.validate_path_segments`` so the + project keeps a single chokepoint for path-segment validation and + also catches single-dot (``.``) traversal segments. + """ + + def test_helpers_traversal_uses_validate_path_segments(self): + """``_expand_with_ancestors`` calls the canonical guard once per + input path. Mocking the guard and asserting it was called proves + the hand-rolled ``".." in parts`` check is gone. + """ + with patch("apm_cli.commands._helpers.validate_path_segments") as mock_guard: + _expand_with_ancestors({"owner/repo/.apm/skills/foo"}) + assert mock_guard.called, ( + "Ancestor expansion must route every input through " + "validate_path_segments rather than a hand-rolled '..' check" + ) + called_paths = {call.args[0] for call in mock_guard.call_args_list} + assert "owner/repo/.apm/skills/foo" in called_paths + + def test_single_dot_segment_now_rejected(self): + """Single-dot segments are rejected by the canonical guard + (which the prior ad-hoc ``".." in parts`` check missed). The + path is kept in the result (membership semantics) but no + ancestors are emitted. + """ + result = _expand_with_ancestors({"owner/./repo"}) + assert "owner/./repo" in result + assert "owner" not in result + assert "owner/." not in result + + +class TestStandaloneInstalledDoesNotSwallowCorruption: + """Round-3 supply-chain finding: bare ``except Exception`` in + ``_standalone_installed_packages`` silently destroyed + ``lockfile_keys`` and failed open on lockfile corruption. The + narrowed ``except (AttributeError, TypeError, KeyError)`` clause + must let unexpected exceptions propagate. + """ + + def test_standalone_installed_does_not_swallow_lockfile_corruption(self, tmp_path): + """A lockfile object whose ``dependencies`` attribute raises an + unexpected exception (e.g. ``RuntimeError`` from a corrupted / + attacker-crafted backing store) must propagate, not silently + return an empty list. + """ + from apm_cli.commands._helpers import _standalone_installed_packages + + class CorruptLockfile: + @property + def dependencies(self): + raise RuntimeError("simulated lockfile corruption") + + with pytest.raises(RuntimeError, match="simulated lockfile corruption"): + _standalone_installed_packages(["owner/repo"], tmp_path, lockfile=CorruptLockfile()) + + def test_standalone_installed_absorbs_narrow_shape_errors(self, tmp_path): + """Narrow shape errors (TypeError when ``dependencies`` is e.g. + an ``int`` due to YAML coercion) are intentionally absorbed and + degrade to the ``apm.yml``-only fallback. + """ + from apm_cli.commands._helpers import _standalone_installed_packages + + class BadShapeLockfile: + dependencies = 42 # not iterable -> TypeError on for-loop + + # Create owner/repo with apm.yml so fallback marks it standalone. + (tmp_path / "owner" / "repo").mkdir(parents=True) + (tmp_path / "owner" / "repo" / "apm.yml").write_text( + "name: r\nversion: 1.0", encoding="utf-8" + ) + result = _standalone_installed_packages( + ["owner/repo"], tmp_path, lockfile=BadShapeLockfile() + ) + assert result == ["owner/repo"], ( + "Narrow shape errors must degrade to apm.yml fallback, not propagate to caller" + ) diff --git a/tests/unit/test_orphan_announce_parity.py b/tests/unit/test_orphan_announce_parity.py new file mode 100644 index 000000000..cf4b2513f --- /dev/null +++ b/tests/unit/test_orphan_announce_parity.py @@ -0,0 +1,160 @@ +"""Round-3 panel regressions: orphan-announce channel parity. + +Both `apm prune` and `apm deps list` surface the same semantic event +("orphans found") and must therefore use the same logger channel: + + * Orphan-found header + per-package bullets -> ``logger.warning`` + (a destructive command must be at least as loud as an advisory + display command, and bullets are subordinate context of that + warning -- not transient progress narration). + * Recovery hint ("Run 'apm prune'...") -> ``logger.info`` + (advisory remediation, not the problem statement; using + ``logger.progress`` risks suppression in ``--quiet`` / CI mode and + silently drops the actionable hint). + +These tests pin the channel mapping so a future refactor cannot +silently regress to ``logger.progress``. +""" + +from __future__ import annotations + +import contextlib +import os +import tempfile +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest +from click.testing import CliRunner + +from apm_cli.cli import cli + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +_APM_YML_NO_DEPS = "name: t\nversion: 1.0.0\ndependencies:\n apm: []\n" + + +def _make_orphan_dir(tmp: Path, owner: str, repo: str) -> Path: + pkg = tmp / "apm_modules" / owner / repo + pkg.mkdir(parents=True) + (pkg / "apm.yml").write_text("name: r\nversion: 1.0.0", encoding="utf-8") + return pkg + + +@contextlib.contextmanager +def _chdir_tmp(): + with tempfile.TemporaryDirectory() as td: + prev = Path.cwd() + os.chdir(td) + try: + yield Path(td) + finally: + os.chdir(prev) + + +def _capture_logger_calls(): + """Return a (logger_factory, calls) tuple. + + The factory is suitable for patching CommandLogger; ``calls`` is a + list of ``(method_name, message)`` tuples in invocation order. + """ + calls: list[tuple[str, str]] = [] + + def _make(*_args, **_kwargs): + logger = MagicMock() + + def _record(name): + def _inner(msg, *a, **k): + calls.append((name, str(msg))) + + return _inner + + for method in ( + "info", + "warning", + "error", + "success", + "progress", + "start", + "debug", + ): + setattr(logger, method, _record(method)) + return logger + + return _make, calls + + +# --------------------------------------------------------------------------- +# Parity: orphan-found header + bullets routed via logger.warning +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "command, patch_target", + [ + (["prune", "--dry-run"], "apm_cli.commands.prune.CommandLogger"), + (["deps", "list"], "apm_cli.commands.deps.cli.CommandLogger"), + ], +) +def test_orphan_announce_level_parity_prune_vs_deps_cli(command, patch_target): + """Both surfaces emit the orphan-found header AND each per-package + bullet through ``logger.warning`` -- never ``logger.progress``. + """ + runner = CliRunner() + factory, calls = _capture_logger_calls() + with _chdir_tmp() as tmp: + (tmp / "apm.yml").write_text(_APM_YML_NO_DEPS) + _make_orphan_dir(tmp, "orphan-org", "orphan-repo") + with patch(patch_target, side_effect=factory): + result = runner.invoke(cli, command) + assert result.exit_code == 0, result.output + + warning_msgs = [m for (lvl, m) in calls if lvl == "warning"] + progress_msgs = [m for (lvl, m) in calls if lvl == "progress"] + + # Header (substring match -- exact text differs slightly between commands) + assert any("orphan" in m.lower() and "package" in m.lower() for m in warning_msgs), ( + f"Orphan-found header must be at warning level. " + f"warnings={warning_msgs!r} progress={progress_msgs!r}" + ) + # Per-package bullet for the orphan must be at warning level too. + assert any("orphan-org/orphan-repo" in m for m in warning_msgs), ( + f"Per-package orphan bullet must be at warning level. " + f"warnings={warning_msgs!r} progress={progress_msgs!r}" + ) + # And must NOT appear at progress level (the regression we are + # pinning closed). + assert not any("orphan-org/orphan-repo" in m for m in progress_msgs), ( + f"Per-package orphan bullet must NOT be emitted at progress " + f"level. progress={progress_msgs!r}" + ) + + +def test_orphan_recovery_hint_uses_info_not_progress(): + """The ``Run 'apm prune' to remove orphaned packages`` hint emitted + by ``apm deps list`` is advisory remediation context. It must use + ``logger.info`` so it survives quiet/CI suppression of the + in-flight ``progress`` channel. + """ + runner = CliRunner() + factory, calls = _capture_logger_calls() + with _chdir_tmp() as tmp: + (tmp / "apm.yml").write_text(_APM_YML_NO_DEPS) + _make_orphan_dir(tmp, "orphan-org", "orphan-repo") + with patch("apm_cli.commands.deps.cli.CommandLogger", side_effect=factory): + result = runner.invoke(cli, ["deps", "list"]) + assert result.exit_code == 0, result.output + + info_msgs = [m for (lvl, m) in calls if lvl == "info"] + progress_msgs = [m for (lvl, m) in calls if lvl == "progress"] + + assert any("apm prune" in m for m in info_msgs), ( + f"Recovery hint must be emitted at info level. " + f"info={info_msgs!r} progress={progress_msgs!r}" + ) + assert not any("apm prune" in m for m in progress_msgs), ( + f"Recovery hint must NOT be emitted at progress level. progress={progress_msgs!r}" + )