diff --git a/docs/src/content/docs/reference/cli/deps.md b/docs/src/content/docs/reference/cli/deps.md index b080dcd44..51eadf2a3 100644 --- a/docs/src/content/docs/reference/cli/deps.md +++ b/docs/src/content/docs/reference/cli/deps.md @@ -32,7 +32,11 @@ All subcommands operate on the project scope (`./apm_modules/`) by default. Pass ### `apm deps list` -List installed dependencies and the primitive counts each one contributes. +List every installed dependency recorded by the manifest or lockfile and the +primitive counts each one contributes. Manifests embedded anywhere inside an +installed package's source tree are parent-owned content, not separate +dependencies. Real lockfile-resolved dependencies install at their own package +roots and remain visible regardless of graph depth. ```bash apm deps list [OPTIONS] @@ -46,7 +50,12 @@ apm deps list [OPTIONS] ### `apm deps tree` -Render the dependency graph as a hierarchical tree, using `apm.lock.yaml` when present and falling back to a scan of `apm_modules/`. +Render the complete dependency graph as a hierarchical tree, following +`resolved_by` relationships at every lockfile depth. When no lockfile is +present, the command falls back to a scan of `apm_modules/` and ignores +parent-owned manifests embedded inside an installed package. Circular +relationships are marked `(circular)` at the repeated ancestor and do not +prevent other branches from rendering. ```bash apm deps tree [OPTIONS] diff --git a/docs/src/content/docs/reference/cli/prune.md b/docs/src/content/docs/reference/cli/prune.md index 181fad19f..e7939a410 100644 --- a/docs/src/content/docs/reference/cli/prune.md +++ b/docs/src/content/docs/reference/cli/prune.md @@ -1,11 +1,13 @@ --- title: apm prune -description: Remove orphaned packages no longer referenced in apm.yml +description: Remove packages absent from the resolved dependency graph sidebar: order: 7 --- -Remove installed packages from `apm_modules/` that are no longer declared in `apm.yml`, along with their deployed integration files. Updates `apm.lock.yaml` to match. +Remove installed packages from `apm_modules/` that are neither declared in +`apm.yml` nor retained as transitive nodes in `apm.lock.yaml`. The command also +removes their deployed integration files and updates the lockfile. ## Synopsis @@ -14,7 +16,10 @@ apm prune [--dry-run] ``` :::note[Coming from npm?] -`apm prune` mirrors `npm prune`: it removes installed packages that are not in your manifest. Unlike npm, it also removes the harness-deployed files those packages produced (prompts, agents, hooks, MCP wiring) and rewrites the lockfile. +`apm prune` mirrors `npm prune`: it removes installed packages that are not in +the manifest's resolved dependency graph. Unlike npm, it also removes the +harness-deployed files those packages produced (prompts, agents, hooks, MCP +wiring) and rewrites the lockfile. ::: ## Description @@ -25,7 +30,12 @@ apm prune [--dry-run] 2. Packages installed under `apm_modules/` 3. Packages recorded in `apm.lock.yaml` with their `deployed_files` -Anything installed but not declared in either dependency list is **orphaned**. `apm prune` removes the orphan's directory under `apm_modules/`, deletes every file the orphan deployed into your harness directories (using the `deployed_files` manifest in the lockfile), removes the entry from `apm.lock.yaml`, and cleans up empty parent directories. +An installed package is **orphaned** when it is neither declared in either +dependency list nor retained as a lockfile-resolved transitive dependency. +`apm prune` removes the orphan's directory under `apm_modules/`, deletes every +file the orphan deployed into your harness directories (using the +`deployed_files` manifest in the lockfile), removes the entry from +`apm.lock.yaml`, and cleans up empty parent directories. If `apm_modules/` does not exist, the command exits cleanly with nothing to do. If `apm.yml` is missing, it exits with an error. @@ -70,6 +80,8 @@ For each orphaned package, `apm prune`: Notes: - Packages that share an install root with a still-declared sibling subdirectory dependency are not falsely protected by ancestor expansion. The check uses lockfile membership (with `apm.yml` fallback) to identify genuine standalone packages. +- A manifest embedded at any depth inside an installed package is owned by that + package. It is not an independent dependency, orphan, or prune candidate. - Deploy paths are validated before deletion; entries that escape the project root are skipped. - The command does not network. It only inspects local state. diff --git a/packages/apm-guide/.apm/skills/apm-usage/commands.md b/packages/apm-guide/.apm/skills/apm-usage/commands.md index f85d77698..ec80280b6 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/commands.md +++ b/packages/apm-guide/.apm/skills/apm-usage/commands.md @@ -13,9 +13,9 @@ | `apm install [PKGS...]` | Install APM and MCP dependencies (supports APM packages, Claude skills (SKILL.md), and plugin collections (plugin.json)) | `--update` (deprecated; prefer `apm update`) refresh refs, `--refresh` re-fetch all deps from upstream and re-resolve all ref pins, `--force` overwrite (does NOT refresh refs; use `apm update` for that), `--frozen` CI-safe install that fails fast when `apm.lock.yaml` is missing or out of sync with `apm.yml` (mutually exclusive with `--update`; structural presence check only -- use `apm audit` for SHA integrity), `--dry-run` (no package/deployment writes; a newly bootstrapped `apm.yml` and explicit targets are kept), `--verbose`, `--only [apm\|mcp]`, `--target` (comma-separated, e.g. `--target claude,cursor`; highest-priority entry in the resolution chain `--target` > apm.yml `targets:` > auto-detect; `intellij` is MCP-only and writes JetBrains Copilot's user-scope config; explicit lists are exact, so `intellij,claude` writes those two MCP configs and `all,intellij` adds JetBrains to `all`; on auto-bootstrap when no `apm.yml` exists, recognized manifest target(s) are persisted to the new manifest's `targets:` so a later bare `apm update` reuses them; `--target all` deprecated, see `apm compile --all`; use `kiro` for Kiro IDE; use `copilot-cowork` with `--global` after `apm experimental enable copilot-cowork`; use `hermes` after `apm experimental enable hermes` to deploy skills + `AGENTS.md` and, at `--global`, MCP servers to `~/.hermes/config.yaml`), `--dev`, `-g` global (MCP deploys only to user-scope runtimes: Copilot CLI, Claude Code, Codex CLI, Gemini CLI, Antigravity CLI, Kiro, Windsurf, JetBrains Copilot, and Hermes when enabled), `--trust-transitive-mcp`, `--parallel-downloads N`, `--allow-insecure`, `--allow-insecure-host HOSTNAME`, `--skill NAME` install named skill(s) from a skill collection (SKILL_BUNDLE or plugin manifest; repeatable; plugin manifests accept a leaf name or manifest path; zero matches fail with available names; persisted in apm.yml only on success; additive across separate installs -- a later `--skill X` adds to the existing pin (union) rather than replacing it, so previously deployed skills are never silently removed; `'*'` resets to the full bundle; drop a single skill by editing the `skills:` list in apm.yml then re-running install), `--legacy-skill-paths` restore per-client skill dirs, `--mcp NAME` add MCP entry (NAME goes through the same `--target` > `targets:` > auto-detect resolver as APM packages, so `apm install --mcp NAME --target intellij` writes only JetBrains Copilot's MCP config; compilation target policy applies to every explicitly selected target; `apm install -g --mcp NAME` writes user-scope and bypasses the project-scope gate by design), `--transport`, `--url`, `--env KEY=VAL`, `--header KEY=VAL`, `--mcp-version`, `--registry URL` custom MCP registry, `--root DIR` redirect writes (`apm_modules/`, lockfile, `.gitignore`, integrated harness files) under DIR while `apm.yml`/`.apm/`/local deps resolve from `$PWD` (mirrors `pip install --target`; created if missing; not valid with `-g`/`--global`, which exits 2). Explicit plugin component paths must resolve inside the plugin root; missing declarations fail before deployment and lockfile commit. | | `apm targets` | Show resolved deployment targets for the current project (Click group; reads filesystem signals; works with or without `apm.yml`) | `--all` also include the `agent-skills` meta-target (only meaningful with `--json`), `--json` machine-readable output. No provenance line is printed (the table is the provenance). | | `apm uninstall PKGS...` | Remove packages (accepts `owner/repo` or `name@marketplace`) | `--dry-run`, `-g` global | -| `apm prune` | Remove orphaned packages | `--dry-run` | -| `apm deps list` | List installed packages | `-g` global, `--all` both scopes, `--insecure` | -| `apm deps tree` | Show dependency tree | -- | +| `apm prune` | Remove installed packages absent from the manifest and lockfile-resolved graph | `--dry-run` | +| `apm deps list` | List manifest- and lockfile-resolved packages; ignore parent-owned embedded manifests | `-g` global, `--all` both scopes, `--insecure` | +| `apm deps tree` | Show the complete lockfile-resolved tree at any depth; mark repeated ancestors as circular | -- | | `apm deps why PKG` | Explain why a package is installed (walks lockfile bottom-up to direct deps; analogue of `npm why` / `yarn why`) | `-g` global, `--json` | | `apm find ` | Trace a deployed file back to the package(s) that contributed it (inverse of install; reads `apm.lock.yaml` only) | `--source` show OCI/git/local origin, `--path` show full why-chain (same as `apm deps why`) | | `apm view PKG [FIELD]` | View package details, git refs, or registry versions | `-g` global, `FIELD=versions`, `--registry [NAME]` forces registry path for versions | diff --git a/src/apm_cli/commands/deps/_utils.py b/src/apm_cli/commands/deps/_utils.py index fcbe5da18..634fae3d2 100644 --- a/src/apm_cli/commands/deps/_utils.py +++ b/src/apm_cli/commands/deps/_utils.py @@ -11,8 +11,10 @@ def _scan_installed_packages(apm_modules_dir: Path) -> list: """Scan *apm_modules_dir* for installed package paths. - Walks the tree to find directories containing ``apm.yml`` or ``.apm``, - supporting GitHub (2-level), ADO (3-level), and subdirectory packages. + Walks the tree to find top-level directories containing ``apm.yml`` or + ``.apm``, supporting GitHub (2-level), ADO (3-level), and subdirectory + packages. Package manifests nested below another package are part of that + parent package and are excluded. Returns: List of ``"owner/repo"`` or ``"org/project/repo"`` path keys. @@ -25,6 +27,8 @@ def _scan_installed_packages(apm_modules_dir: Path) -> list: continue if not ((candidate / APM_YML_FILENAME).exists() or (candidate / APM_DIR).exists()): continue + if _is_nested_under_package(candidate, apm_modules_dir): + continue rel_parts = candidate.relative_to(apm_modules_dir).parts if len(rel_parts) >= 2: installed.append("/".join(rel_parts)) @@ -34,16 +38,16 @@ def _scan_installed_packages(apm_modules_dir: Path) -> list: def _is_nested_under_package(candidate: Path, apm_modules_path: Path) -> bool: """Check if *candidate* is a sub-directory of another installed package. - When a plugin ships ``skills/*/SKILL.md`` at its root (outside ``.apm/``), - the ``rglob`` scan would otherwise treat each skill sub-directory as an - independent package. This helper walks up from *candidate* towards - *apm_modules_path* and returns ``True`` if any intermediate parent already - contains ``apm.yml`` -- meaning the candidate is a deployment artifact, not - a standalone package. + When a package ships nested package or skill manifests, the ``rglob`` scan + would otherwise treat each sub-directory as an independent package. This + helper walks up from *candidate* towards *apm_modules_path* and returns + ``True`` if any intermediate parent already contains ``apm.yml`` or + ``.apm`` -- meaning the candidate is a deployment artifact, not a standalone + package. """ parent = candidate.parent while parent != apm_modules_path and parent != parent.parent: # noqa: PLR1714 - if (parent / APM_YML_FILENAME).exists(): + if (parent / APM_YML_FILENAME).exists() or (parent / APM_DIR).exists(): return True parent = parent.parent return False diff --git a/src/apm_cli/commands/deps/cli.py b/src/apm_cli/commands/deps/cli.py index 03f2cbfb6..731299b4e 100644 --- a/src/apm_cli/commands/deps/cli.py +++ b/src/apm_cli/commands/deps/cli.py @@ -2,6 +2,7 @@ import shutil import sys +from collections.abc import Iterator from pathlib import Path import click @@ -10,6 +11,7 @@ from ...constants import APM_MODULES_DIR, APM_YML_FILENAME, SKILL_MD_FILENAME from ...core.command_logger import CommandLogger from ...core.target_detection import TargetParamType +from ...deps.lockfile import LockedDependency from ...models.apm_package import APMPackage from .._helpers import ( UnknownPackageError, @@ -70,42 +72,71 @@ def _dep_display_name(dep) -> str: return f"{key}@{version}" -def _add_tree_children(parent_branch, parent_key, children_map, has_rich, depth=0): - """Recursively add transitive deps as nested children of a tree node.""" - kids = children_map.get(parent_key, []) - for child_dep in kids: - child_name = _dep_display_name(child_dep) - child_branch = parent_branch.add(f"[dim]{child_name}[/dim]") if has_rich else child_name - if depth < 5: # Prevent infinite recursion - _add_tree_children( - child_branch, - child_dep.get_unique_key(), - children_map, - has_rich, - depth + 1, +def _walk_tree_children( + parent_key: str, + children_map: dict[str, list[LockedDependency]], +) -> Iterator[tuple[LockedDependency, tuple[int, ...], tuple[bool, ...], bool]]: + """Yield dependency-tree descendants depth-first without Python recursion.""" + ancestors = frozenset({parent_key}) + children = children_map.get(parent_key, []) + stack: list[tuple[LockedDependency, tuple[int, ...], tuple[bool, ...], frozenset[str]]] = [] + for index in range(len(children) - 1, -1, -1): + stack.append( + ( + children[index], + (index,), + (index == len(children) - 1,), + ancestors, + ) + ) + + while stack: + child_dep, path, last_flags, parent_ancestors = stack.pop() + child_key = child_dep.get_unique_key() + is_circular = child_key in parent_ancestors + yield child_dep, path, last_flags, is_circular + if is_circular: + continue + + child_ancestors = parent_ancestors | {child_key} + grandchildren = children_map.get(child_key, []) + for index in range(len(grandchildren) - 1, -1, -1): + stack.append( + ( + grandchildren[index], + (*path, index), + (*last_flags, index == len(grandchildren) - 1), + child_ancestors, + ) ) +def _add_tree_children( + parent_branch, + parent_key: str, + children_map: dict[str, list[LockedDependency]], +) -> None: + """Add every transitive dependency to a Rich tree without a depth limit.""" + branches = {(): parent_branch} + for child_dep, path, _, is_circular in _walk_tree_children(parent_key, children_map): + child_name = _dep_display_name(child_dep) + suffix = " (circular)" if is_circular else "" + child_branch = branches[path[:-1]].add(f"[dim]{child_name}{suffix}[/dim]") + if not is_circular: + branches[path] = child_branch + + def _echo_tree_children( parent_key: str, - children_map: dict[str, list], + children_map: dict[str, list[LockedDependency]], prefix: str = "", - depth: int = 0, ) -> None: - """Render transitive dependencies recursively without Rich.""" - kids = children_map.get(parent_key, []) - for index, child_dep in enumerate(kids): - is_last = index == len(kids) - 1 - child_prefix = "+-- " if is_last else "|-- " - click.echo(f"{prefix}{child_prefix}{_dep_display_name(child_dep)}") - if depth < 5: - continuation = " " if is_last else "| " - _echo_tree_children( - child_dep.get_unique_key(), - children_map, - prefix + continuation, - depth + 1, - ) + """Render every transitive dependency without Rich or a depth limit.""" + for child_dep, _, last_flags, is_circular in _walk_tree_children(parent_key, children_map): + indentation = "".join(" " if is_last else "| " for is_last in last_flags[:-1]) + child_prefix = "+-- " if last_flags[-1] else "|-- " + suffix = " (circular)" if is_circular else "" + click.echo(f"{prefix}{indentation}{child_prefix}{_dep_display_name(child_dep)}{suffix}") # --------------------------------------------------------------------------- @@ -221,13 +252,8 @@ def _resolve_scope_deps(apm_dir, logger, insecure_only=False): if ".apm" in rel_parts: continue - # Skip skill sub-dirs nested inside another package (e.g. plugin - # skills/ directories that are deployment artifacts, not packages). - if ( - has_skill_md - and not has_apm_yml - and _is_nested_under_package(candidate, apm_modules_path) - ): + # Nested manifests are deployment artifacts owned by the parent package. + if _is_nested_under_package(candidate, apm_modules_path): continue scanned_candidates.append((candidate, "/".join(rel_parts), has_apm_yml, has_skill_md)) @@ -368,7 +394,10 @@ def _show_scope_deps(scope_label, apm_dir, logger, console, has_rich, insecure_o # (no raw `[!]` literal that Rich would parse as markup) and so # behaviour is consistent with prune.py. if orphaned_packages: - logger.warning(f"{len(orphaned_packages)} orphaned package(s) found (not in apm.yml):") + logger.warning( + f"{len(orphaned_packages)} orphaned package(s) found " + "(not in resolved dependency graph):" + ) for pkg in orphaned_packages: logger.warning(f" - {pkg}") logger.info("Run 'apm prune' to remove orphaned packages") @@ -412,13 +441,16 @@ def _show_scope_deps(scope_label, apm_dir, logger, console, has_rich, insecure_o # Show orphaned packages warning -- route through CommandLogger # for consistency with the rich branch above and with prune.py. if orphaned_packages: - logger.warning(f"{len(orphaned_packages)} orphaned package(s) found (not in apm.yml):") + logger.warning( + f"{len(orphaned_packages)} orphaned package(s) found " + "(not in resolved dependency graph):" + ) for pkg in orphaned_packages: logger.warning(f" - {pkg}") logger.info("Run 'apm prune' to remove orphaned packages") -@deps.command(name="list", help="List installed APM dependencies") +@deps.command(name="list", help="List installed APM dependencies and their primitives") @click.option( "--global", "-g", @@ -561,10 +593,10 @@ def _build_dep_tree(apm_dir): result["source"] = "lockfile" result["direct"] = [d for d in lockfile_deps if d.depth <= 1] transitive = [d for d in lockfile_deps if d.depth > 1] - children_map: dict[str, list] = {} + children_map: dict[str, list[LockedDependency]] = {} unresolved = [] - parents_by_unique_key: dict[tuple[int, str], list] = {} - parents_by_repo_url: dict[tuple[int, str], list] = {} + parents_by_unique_key: dict[tuple[int, str], list[LockedDependency]] = {} + parents_by_repo_url: dict[tuple[int, str], list[LockedDependency]] = {} for candidate in lockfile_deps: parents_by_unique_key.setdefault( (candidate.depth, candidate.get_unique_key()), [] @@ -613,7 +645,7 @@ def _build_dep_tree(apm_dir): continue if ".apm" in rel_parts: continue - if has_skill and not has_apm and _is_nested_under_package(candidate, apm_modules_path): + if _is_nested_under_package(candidate, apm_modules_path): continue info = _get_package_display_info(candidate) primitives = _count_primitives(candidate) @@ -627,7 +659,7 @@ def _build_dep_tree(apm_dir): return result -@deps.command(help="Show dependency tree structure") +@deps.command(help="Show the full dependency tree") @click.option( "--global", "-g", @@ -680,13 +712,14 @@ def tree(global_): prim_summary = _format_primitive_counts(_count_primitives(install_path)) if prim_summary: branch.add(f"[dim]{prim_summary}[/dim]") - _add_tree_children(branch, dep.get_unique_key(), children_map, has_rich) + _add_tree_children(branch, dep.get_unique_key(), children_map) for dep in unresolved: display = _dep_display_name(dep) branch = root_tree.add( - f"[yellow]{display} (could not determine parent)[/yellow]" + f"[yellow]{display} (could not place in tree; " + "run apm install to resolve)[/yellow]" ) - _add_tree_children(branch, dep.get_unique_key(), children_map, has_rich) + _add_tree_children(branch, dep.get_unique_key(), children_map) console.print(root_tree) else: click.echo(f"{project_name} (local)") @@ -703,7 +736,11 @@ def tree(global_): for i, dep in enumerate(unresolved): is_last = i == len(unresolved) - 1 prefix = "+-- " if is_last else "|-- " - click.echo(f"{prefix}{_dep_display_name(dep)} (could not determine parent)") + click.echo( + f"{prefix}{_dep_display_name(dep)} " + "(could not place in tree; " + "run apm install to resolve)" + ) sub_prefix = " " if is_last else "| " _echo_tree_children(dep.get_unique_key(), children_map, sub_prefix) # Fallback: scan apm_modules directory (no lockfile) diff --git a/src/apm_cli/commands/prune.py b/src/apm_cli/commands/prune.py index 549131cee..70c7898b2 100644 --- a/src/apm_cli/commands/prune.py +++ b/src/apm_cli/commands/prune.py @@ -21,14 +21,14 @@ ) -@click.command(help="Remove APM packages not listed in apm.yml") +@click.command(help="Remove APM packages absent from the resolved dependency graph") @click.option("--dry-run", is_flag=True, help="Show what would be removed without removing") @click.pass_context def prune(ctx, dry_run): - """Remove installed APM packages that are not listed in apm.yml (like npm prune). + """Remove installed APM packages absent from the resolved dependency graph (like npm prune). - This command cleans up the apm_modules/ directory by removing packages that - were previously installed but are no longer declared as dependencies in apm.yml. + This command cleans up the apm_modules/ directory by removing packages that are + neither declared in apm.yml nor retained as transitive nodes in apm.lock.yaml. Examples: apm prune # Remove orphaned packages diff --git a/tests/integration/test_deps_cli_coverage.py b/tests/integration/test_deps_cli_coverage.py index d2e3bc961..2430c95b8 100644 --- a/tests/integration/test_deps_cli_coverage.py +++ b/tests/integration/test_deps_cli_coverage.py @@ -189,13 +189,13 @@ def test_display_name_version_takes_precedence(self): class TestAddTreeChildren: """Tests for _add_tree_children helper.""" - def test_add_children_no_rich(self): - """Add children without Rich console.""" + def test_add_children_empty_map(self): + """An empty child map leaves the Rich branch unchanged.""" parent_branch = MagicMock() parent_repo_url = "https://github.com/owner/parent" children_map = {} - _add_tree_children(parent_branch, parent_repo_url, children_map, False) + _add_tree_children(parent_branch, parent_repo_url, children_map) # No children, so parent_branch.add should not be called parent_branch.add.assert_not_called() @@ -217,7 +217,7 @@ def test_add_single_child(self): children_map = {parent_repo_url: [mock_child]} - _add_tree_children(parent_branch, parent_repo_url, children_map, True) + _add_tree_children(parent_branch, parent_repo_url, children_map) # Parent should have added child parent_branch.add.assert_called_once() @@ -247,17 +247,18 @@ def test_add_multiple_children(self): children_map = {parent_repo_url: [mock_child1, mock_child2]} - _add_tree_children(parent_branch, parent_repo_url, children_map, True) + _add_tree_children(parent_branch, parent_repo_url, children_map) # Parent should have added both children assert parent_branch.add.call_count == 2 - def test_add_children_respects_depth_limit(self): - """Recursive depth is limited to 5.""" + def test_add_children_stops_at_cycle(self): + """Recursive rendering stops when a dependency repeats an ancestor.""" parent_branch = MagicMock() + child_branch = MagicMock() + parent_branch.add.return_value = child_branch parent_repo_url = "https://github.com/owner/parent" - # Create a chain of dependencies beyond depth limit mock_child = MagicMock() mock_child.get_unique_key.return_value = "owner/child" mock_child.version = "1.0.0" @@ -265,19 +266,22 @@ def test_add_children_respects_depth_limit(self): mock_child.resolved_ref = None mock_child.repo_url = "https://github.com/owner/child" - children_map = {parent_repo_url: [mock_child]} + repeated_parent = MagicMock() + repeated_parent.get_unique_key.return_value = parent_repo_url + repeated_parent.version = "1.0.0" + repeated_parent.resolved_commit = None + repeated_parent.resolved_ref = None + repeated_parent.repo_url = parent_repo_url + children_map = { + parent_repo_url: [mock_child], + "owner/child": [repeated_parent], + } - # Call at depth 5 (max allowed) - _add_tree_children( - parent_branch, - parent_repo_url, - children_map, - True, - depth=5, - ) + _add_tree_children(parent_branch, parent_repo_url, children_map) - # At max depth, children should not be added recursively - # (but the initial child is still added) + parent_branch.add.assert_called_once() + child_branch.add.assert_called_once() + assert "(circular)" in child_branch.add.call_args.args[0] def test_add_children_with_no_children_map_entry(self): """No children for parent means nothing is added.""" @@ -285,7 +289,7 @@ def test_add_children_with_no_children_map_entry(self): parent_repo_url = "https://github.com/owner/parent" children_map = {} - _add_tree_children(parent_branch, parent_repo_url, children_map, True) + _add_tree_children(parent_branch, parent_repo_url, children_map) parent_branch.add.assert_not_called() diff --git a/tests/integration/test_intra_package_cleanup.py b/tests/integration/test_intra_package_cleanup.py index 7405015b9..4404877d1 100644 --- a/tests/integration/test_intra_package_cleanup.py +++ b/tests/integration/test_intra_package_cleanup.py @@ -149,6 +149,70 @@ def _make_local_prompt_package(tmp_path, name, prompts): return pkg +def _make_local_nested_package(tmp_path, name): + """Create a local package whose source includes a nested package manifest.""" + pkg = tmp_path / name + nested = pkg / "sub-package" + nested.mkdir(parents=True) + (pkg / "apm.yml").write_text( + yaml.dump({"name": name, "version": "0.0.1"}, default_flow_style=False), + encoding="utf-8", + ) + (nested / "apm.yml").write_text( + yaml.dump({"name": "sub-package", "version": "0.0.1"}, default_flow_style=False), + encoding="utf-8", + ) + (nested / "payload.txt").write_text("nested package content\n", encoding="utf-8") + return pkg + + +class TestNestedPackagePrune: + """End-to-end regression coverage for nested package ownership.""" + + def test_prune_preserves_nested_content_then_removes_whole_parent( + self, temp_project, apm_command, tmp_path + ): + """Prune protects a declared parent, then removes all of it once orphaned.""" + parent = _make_local_nested_package(tmp_path, "parent-pkg") + unrelated = _make_local_nested_package(tmp_path, "unrelated-pkg") + _write_apm_yml_local_packages(temp_project, [parent, unrelated]) + + installed = _run_apm(apm_command, ["install"], temp_project) + assert installed.returncode == 0, ( + f"Install failed:\nSTDOUT: {installed.stdout}\nSTDERR: {installed.stderr}" + ) + + parent_install = temp_project / "apm_modules" / "_local" / "parent-pkg" + unrelated_install = temp_project / "apm_modules" / "_local" / "unrelated-pkg" + nested_payload = parent_install / "sub-package" / "payload.txt" + unrelated_payload = unrelated_install / "sub-package" / "payload.txt" + assert nested_payload.is_file() + assert unrelated_payload.is_file() + + updated = _run_apm(apm_command, ["update", "--yes"], temp_project) + assert updated.returncode == 0, ( + f"Update failed:\nSTDOUT: {updated.stdout}\nSTDERR: {updated.stderr}" + ) + assert nested_payload.is_file(), "Update removed nested package content" + assert unrelated_payload.is_file(), "Update removed unrelated package content" + + kept = _run_apm(apm_command, ["prune"], temp_project) + assert kept.returncode == 0, ( + f"Initial prune failed:\nSTDOUT: {kept.stdout}\nSTDERR: {kept.stderr}" + ) + assert nested_payload.is_file(), "Nested package content was pruned from its parent" + assert unrelated_payload.is_file(), "Unrelated package content was pruned" + + _write_apm_yml_local_packages(temp_project, [unrelated]) + removed = _run_apm(apm_command, ["prune"], temp_project) + assert removed.returncode == 0, ( + f"Orphan prune failed:\nSTDOUT: {removed.stdout}\nSTDERR: {removed.stderr}" + ) + + assert not parent_install.exists(), "Orphaned parent package was only partially pruned" + assert unrelated_payload.is_file(), "Pruning the parent deleted unrelated material" + + class TestFileRenamedWithinPackage: """Regression tests for issue #666: renaming a file inside a still-present package must delete the stale deployed artifacts on the next apm install.""" diff --git a/tests/integration/test_transitive_chain_e2e.py b/tests/integration/test_transitive_chain_e2e.py index 2a9c4c984..ee6d8b9ed 100644 --- a/tests/integration/test_transitive_chain_e2e.py +++ b/tests/integration/test_transitive_chain_e2e.py @@ -9,12 +9,16 @@ import shutil import subprocess +from itertools import pairwise from pathlib import Path import pytest import yaml +pytestmark = pytest.mark.requires_apm_binary + TIMEOUT = 180 +DEEP_CHAIN_LENGTH = 8 @pytest.fixture @@ -43,6 +47,19 @@ def _write_pkg(pkg_dir: Path, name: str, deps: list, primitive_name: str) -> Non ) +def _write_embedded_manifest(package: Path, relative_dir: str, name: str) -> Path: + """Add parent-owned source content that resembles an APM package.""" + embedded = package / relative_dir + embedded.mkdir(parents=True) + (embedded / "apm.yml").write_text( + yaml.dump({"name": name, "version": "1.0.0"}), + encoding="utf-8", + ) + payload = embedded / "payload.txt" + payload.write_text(f"{name} content\n", encoding="utf-8") + return payload + + @pytest.fixture def chain_workspace(tmp_path): """Build workspace/{consumer, pkg-a, pkg-b, pkg-c} with a 3-level chain.""" @@ -73,6 +90,41 @@ def chain_workspace(tmp_path): return workspace +@pytest.fixture +def deep_chain_workspace(tmp_path: Path) -> Path: + """Build a local eight-package chain with embedded parent-owned manifests.""" + workspace = tmp_path / "deep-workspace" + workspace.mkdir() + consumer = workspace / "consumer" + consumer.mkdir() + (consumer / ".github").mkdir() + (consumer / "apm.yml").write_text( + yaml.dump( + { + "name": "deep-consumer", + "version": "1.0.0", + "target": "copilot", + "dependencies": {"apm": []}, + } + ), + encoding="utf-8", + ) + + package_names = [f"pkg-depth-{depth}" for depth in range(1, DEEP_CHAIN_LENGTH + 1)] + for index, name in enumerate(package_names): + dependencies = [f"../{package_names[index + 1]}"] if index + 1 < len(package_names) else [] + _write_pkg(workspace / name, name, dependencies, f"primitive-{index + 1}") + + root_package = workspace / package_names[0] + _write_embedded_manifest(root_package, "examples/embedded-shallow", "embedded-shallow") + _write_embedded_manifest( + root_package, + "vendor/fixtures/nested/embedded-deep", + "embedded-deep", + ) + return workspace + + def _load_lockfile(consumer: Path) -> dict: lock_path = consumer / "apm.lock.yaml" assert lock_path.exists(), "Lockfile not created" @@ -133,6 +185,100 @@ def test_three_level_apm_chain_resolves_all_levels(chain_workspace, apm_command) ) +def test_deps_commands_follow_full_lock_graph_and_ignore_embedded_manifests( + deep_chain_workspace: Path, + apm_command: str, +) -> None: + """CLI inventory follows every resolved edge but ignores parent-owned manifests.""" + consumer = deep_chain_workspace / "consumer" + package_names = [f"pkg-depth-{depth}" for depth in range(1, DEEP_CHAIN_LENGTH + 1)] + package_keys = [f"_local/{name}" for name in package_names] + tree_keys = [f"../{name}" for name in package_names] + embedded_names = ("embedded-shallow", "embedded-deep") + + installed = subprocess.run( + [apm_command, "install", f"../{package_names[0]}"], + cwd=consumer, + capture_output=True, + text=True, + timeout=TIMEOUT, + ) + assert installed.returncode == 0, f"Install failed: {installed.stderr}\n{installed.stdout}" + + locked = _deps_by_name(_load_lockfile(consumer)) + assert list(locked) == package_keys + for depth, key in enumerate(package_keys, start=1): + assert locked[key].get("depth", 1) == depth + expected_parent = package_keys[depth - 2] if depth > 1 else None + assert locked[key].get("resolved_by") in (expected_parent, "") + + listed = subprocess.run( + [apm_command, "deps", "list"], + cwd=consumer, + capture_output=True, + text=True, + timeout=TIMEOUT, + ) + assert listed.returncode == 0, f"deps list failed: {listed.stderr}\n{listed.stdout}" + list_output = listed.stdout + listed.stderr + for key in package_keys: + assert key in list_output + for embedded_name in embedded_names: + assert embedded_name not in list_output + assert "orphaned package(s) found" not in list_output + + tree = subprocess.run( + [apm_command, "deps", "tree"], + cwd=consumer, + capture_output=True, + text=True, + timeout=TIMEOUT, + ) + assert tree.returncode == 0, f"deps tree failed: {tree.stderr}\n{tree.stdout}" + tree_output = tree.stdout + tree.stderr + tree_lines = [line for line in tree_output.splitlines() if "../pkg-depth-" in line] + assert len(tree_lines) == DEEP_CHAIN_LENGTH, tree_output + assert all(key in line for key, line in zip(tree_keys, tree_lines, strict=True)) + key_columns = [line.index(key) for key, line in zip(tree_keys, tree_lines, strict=True)] + assert all(left < right for left, right in pairwise(key_columns)) + for embedded_name in embedded_names: + assert embedded_name not in tree_output + + pruned = subprocess.run( + [apm_command, "prune"], + cwd=consumer, + capture_output=True, + text=True, + timeout=TIMEOUT, + ) + assert pruned.returncode == 0, f"prune failed: {pruned.stderr}\n{pruned.stdout}" + prune_output = pruned.stdout + pruned.stderr + for embedded_name in embedded_names: + assert embedded_name not in prune_output + for package_name in package_names: + assert (consumer / "apm_modules" / "_local" / package_name / "apm.yml").is_file() + assert ( + consumer + / "apm_modules" + / "_local" + / package_names[0] + / "examples" + / "embedded-shallow" + / "payload.txt" + ).is_file() + assert ( + consumer + / "apm_modules" + / "_local" + / package_names[0] + / "vendor" + / "fixtures" + / "nested" + / "embedded-deep" + / "payload.txt" + ).is_file() + + def test_three_level_chain_uninstall_root_cascades(chain_workspace, apm_command): """Uninstalling the root drops orphaned transitive deps and their primitives.""" consumer = chain_workspace / "consumer" diff --git a/tests/integration/test_wave6_deps_cli_coverage.py b/tests/integration/test_wave6_deps_cli_coverage.py index 0b6ddc0a4..97d135c3c 100644 --- a/tests/integration/test_wave6_deps_cli_coverage.py +++ b/tests/integration/test_wave6_deps_cli_coverage.py @@ -476,7 +476,8 @@ def test_tree_surfaces_dependency_with_ambiguous_legacy_parent( assert result.exit_code == 0, result.output assert result.output.count("example/child@1.0.0") == 1 - assert "could not determine parent" in result.output + assert "could not place in tree" in result.output + assert "run apm install" in result.output def test_tree_no_lockfile_with_modules( self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch diff --git a/tests/unit/commands/test_deps_cli_cli_surface.py b/tests/unit/commands/test_deps_cli_cli_surface.py index 397a33335..29f594c14 100644 --- a/tests/unit/commands/test_deps_cli_cli_surface.py +++ b/tests/unit/commands/test_deps_cli_cli_surface.py @@ -22,6 +22,7 @@ _add_tree_children, _build_dep_tree, _deps_list_source_label, + _echo_tree_children, _resolve_scope_deps, _show_scope_deps, ) @@ -68,6 +69,15 @@ def _make_dep( return dep +class _RecordingBranch: + def __init__(self, labels: list[str]) -> None: + self.labels = labels + + def add(self, label: str) -> _RecordingBranch: + self.labels.append(label) + return _RecordingBranch(self.labels) + + def _make_pkg_dict( name: str = "owner/repo", version: str = "1.0.0", @@ -133,41 +143,54 @@ def test_none_host_returns_github(self) -> None: class TestAddTreeChildren: def test_no_children_does_nothing(self) -> None: parent = MagicMock() - _add_tree_children(parent, "owner/repo", {}, has_rich=False) + _add_tree_children(parent, "owner/repo", {}) parent.add.assert_not_called() - def test_adds_child_dep_names_no_rich(self) -> None: - """With has_rich=False the parent branch add is not called (strings are used).""" - child = _make_dep("child/repo", version="1.0.0", repo_url="child/repo") - parent = MagicMock() - children_map = {"owner/repo": [child]} - # has_rich=False: child_branch is a string, parent.add is NOT called - _add_tree_children(parent, "owner/repo", children_map, has_rich=False) - parent.add.assert_not_called() - - def test_adds_child_dep_names_with_rich(self) -> None: - """With has_rich=True the parent branch .add() is called.""" + def test_adds_child_dep_names(self) -> None: + """The Rich parent branch receives each child.""" child = _make_dep("child/repo", version="1.0.0", repo_url="child/repo") parent = MagicMock() child_branch = MagicMock() parent.add.return_value = child_branch children_map = {"owner/repo": [child]} - _add_tree_children(parent, "owner/repo", children_map, has_rich=True) + _add_tree_children(parent, "owner/repo", children_map) parent.add.assert_called_once() - def test_depth_limit_prevents_infinite_recursion(self) -> None: - """Depth ≥ 5 should not recurse further.""" + def test_cycle_guard_marks_repeated_ancestor(self) -> None: + """A dependency cycle is marked at the first repeated ancestor.""" child = _make_dep("child/repo", version="1.0.0", repo_url="child/repo") - grandchild = _make_dep("grand/repo", version="1.0.0", repo_url="grand/repo") + repeated_parent = _make_dep("owner/repo", version="1.0.0", repo_url="owner/repo") children_map = { "owner/repo": [child], - "child/repo": [grandchild], + "child/repo": [repeated_parent], } parent = MagicMock() - # Call at depth=5 -- should add child but NOT recurse into grandchild - _add_tree_children(parent, "owner/repo", children_map, has_rich=False, depth=5) - # The child should be added (we're at depth=5, which is >= 5, so NOT recursed) - parent.add.assert_not_called() + child_branch = MagicMock() + parent.add.return_value = child_branch + + _add_tree_children(parent, "owner/repo", children_map) + + parent.add.assert_called_once() + child_branch.add.assert_called_once() + assert "(circular)" in child_branch.add.call_args.args[0] + + def test_renders_chain_beyond_python_recursion_limit(self) -> None: + """A valid lock graph is rendered without a call-stack depth limit.""" + chain_length = 1100 + children_map = { + f"pkg-{index}": [_make_dep(f"pkg-{index + 1}", version="1.0.0")] + for index in range(chain_length) + } + labels: list[str] = [] + + _add_tree_children( + _RecordingBranch(labels), + "pkg-0", + children_map, + ) + + assert len(labels) == chain_length + assert "pkg-1100@1.0.0" in labels[-1] def test_has_rich_uses_rich_markup(self) -> None: child = _make_dep("child/repo", version="1.0.0", repo_url="child/repo") @@ -175,12 +198,45 @@ def test_has_rich_uses_rich_markup(self) -> None: child_branch = MagicMock() parent.add.return_value = child_branch children_map = {"owner/repo": [child]} - _add_tree_children(parent, "owner/repo", children_map, has_rich=True) - # With has_rich=True the child name is wrapped in [dim] markup + _add_tree_children(parent, "owner/repo", children_map) call_args = parent.add.call_args[0][0] assert "[dim]" in call_args +class TestEchoTreeChildren: + def test_cycle_is_visible_and_stops_traversal(self) -> None: + """The text fallback marks a repeated ancestor without looping.""" + child = _make_dep("child/repo", version="1.0.0") + repeated_parent = _make_dep("owner/repo", version="1.0.0") + children_map = { + "owner/repo": [child], + "child/repo": [repeated_parent], + } + + with patch("apm_cli.commands.deps.cli.click.echo") as echo: + _echo_tree_children("owner/repo", children_map) + + assert echo.call_count == 2 + assert "(circular)" in echo.call_args_list[-1].args[0] + + def test_nested_children_use_ascii_tree_prefixes(self) -> None: + """The text fallback preserves nesting with portable ASCII prefixes.""" + child = _make_dep("child/repo", version="1.0.0") + grandchild = _make_dep("grandchild/repo", version="1.0.0") + children_map = { + "owner/repo": [child], + "child/repo": [grandchild], + } + + with patch("apm_cli.commands.deps.cli.click.echo") as echo: + _echo_tree_children("owner/repo", children_map) + + assert [call.args[0] for call in echo.call_args_list] == [ + "+-- child/repo@1.0.0", + " +-- grandchild/repo@1.0.0", + ] + + # --------------------------------------------------------------------------- # _show_scope_deps # --------------------------------------------------------------------------- diff --git a/tests/unit/commands/test_deps_cli_helpers.py b/tests/unit/commands/test_deps_cli_helpers.py index 7a05f97a0..7fada86db 100644 --- a/tests/unit/commands/test_deps_cli_helpers.py +++ b/tests/unit/commands/test_deps_cli_helpers.py @@ -159,3 +159,18 @@ def test_skill_md_only_package_discovered(self, tmp_path): installed, _orphaned = _resolve_scope_deps(tmp_path, _make_logger()) assert installed is not None assert any(pkg["name"].endswith("skill-repo") for pkg in installed) + + def test_nested_package_manifest_is_not_listed_independently(self, tmp_path): + """A nested manifest is omitted while its undeclared parent is orphaned.""" + modules_dir = tmp_path / APM_MODULES_DIR + parent = modules_dir / "_local" / "package" + nested = parent / "sub-package" + nested.mkdir(parents=True) + (parent / APM_YML_FILENAME).write_text("name: package\nversion: 1.0.0\n") + (nested / APM_YML_FILENAME).write_text("name: sub-package\nversion: 1.0.0\n") + + installed, orphaned = _resolve_scope_deps(tmp_path, _make_logger()) + + assert installed is not None + assert [package["name"] for package in installed] == ["_local/package"] + assert orphaned == ["_local/package"] diff --git a/tests/unit/commands/test_deps_cli_phase3.py b/tests/unit/commands/test_deps_cli_phase3.py index a14f868be..1412fceac 100644 --- a/tests/unit/commands/test_deps_cli_phase3.py +++ b/tests/unit/commands/test_deps_cli_phase3.py @@ -133,41 +133,36 @@ def test_none_host_returns_github(self) -> None: class TestAddTreeChildren: def test_no_children_does_nothing(self) -> None: parent = MagicMock() - _add_tree_children(parent, "owner/repo", {}, has_rich=False) + _add_tree_children(parent, "owner/repo", {}) parent.add.assert_not_called() - def test_adds_child_dep_names_no_rich(self) -> None: - """With has_rich=False the parent branch add is not called (strings are used).""" - child = _make_dep("child/repo", version="1.0.0", repo_url="child/repo") - parent = MagicMock() - children_map = {"owner/repo": [child]} - # has_rich=False: child_branch is a string, parent.add is NOT called - _add_tree_children(parent, "owner/repo", children_map, has_rich=False) - parent.add.assert_not_called() - - def test_adds_child_dep_names_with_rich(self) -> None: - """With has_rich=True the parent branch .add() is called.""" + def test_adds_child_dep_names(self) -> None: + """The Rich parent branch receives each child.""" child = _make_dep("child/repo", version="1.0.0", repo_url="child/repo") parent = MagicMock() child_branch = MagicMock() parent.add.return_value = child_branch children_map = {"owner/repo": [child]} - _add_tree_children(parent, "owner/repo", children_map, has_rich=True) + _add_tree_children(parent, "owner/repo", children_map) parent.add.assert_called_once() - def test_depth_limit_prevents_infinite_recursion(self) -> None: - """Depth ≥ 5 should not recurse further.""" + def test_cycle_guard_marks_repeated_ancestor(self) -> None: + """A dependency cycle is marked at the first repeated ancestor.""" child = _make_dep("child/repo", version="1.0.0", repo_url="child/repo") - grandchild = _make_dep("grand/repo", version="1.0.0", repo_url="grand/repo") + repeated_parent = _make_dep("owner/repo", version="1.0.0", repo_url="owner/repo") children_map = { "owner/repo": [child], - "child/repo": [grandchild], + "child/repo": [repeated_parent], } parent = MagicMock() - # Call at depth=5 -- should add child but NOT recurse into grandchild - _add_tree_children(parent, "owner/repo", children_map, has_rich=False, depth=5) - # The child should be added (we're at depth=5, which is >= 5, so NOT recursed) - parent.add.assert_not_called() + child_branch = MagicMock() + parent.add.return_value = child_branch + + _add_tree_children(parent, "owner/repo", children_map) + + parent.add.assert_called_once() + child_branch.add.assert_called_once() + assert "(circular)" in child_branch.add.call_args.args[0] def test_has_rich_uses_rich_markup(self) -> None: child = _make_dep("child/repo", version="1.0.0", repo_url="child/repo") @@ -175,8 +170,7 @@ def test_has_rich_uses_rich_markup(self) -> None: child_branch = MagicMock() parent.add.return_value = child_branch children_map = {"owner/repo": [child]} - _add_tree_children(parent, "owner/repo", children_map, has_rich=True) - # With has_rich=True the child name is wrapped in [dim] markup + _add_tree_children(parent, "owner/repo", children_map) call_args = parent.add.call_args[0][0] assert "[dim]" in call_args diff --git a/tests/unit/commands/test_deps_cli_phase3w4.py b/tests/unit/commands/test_deps_cli_phase3w4.py index b814e6460..f115da8fd 100644 --- a/tests/unit/commands/test_deps_cli_phase3w4.py +++ b/tests/unit/commands/test_deps_cli_phase3w4.py @@ -398,6 +398,22 @@ def test_apm_in_rel_parts_skipped(self, tmp_path: Path) -> None: names = [p["display_name"] for p in result["scanned_packages"]] assert not any(".apm" in n for n in names) + def test_nested_package_manifest_skipped(self, tmp_path: Path) -> None: + """A nested apm.yml is owned by its parent and omitted from the tree.""" + modules = tmp_path / APM_MODULES_DIR + parent = modules / "_local" / "package" + nested = parent / "sub-package" + nested.mkdir(parents=True) + (parent / APM_YML_FILENAME).write_text("name: package\nversion: 1.0.0\n", encoding="utf-8") + (nested / APM_YML_FILENAME).write_text( + "name: sub-package\nversion: 1.0.0\n", encoding="utf-8" + ) + + result = _build_dep_tree(tmp_path) + + names = [package["display_name"] for package in result["scanned_packages"]] + assert names == ["package@1.0.0"] + # --------------------------------------------------------------------------- # deps tree command -- non-rich lockfile source (lines 612-627) diff --git a/tests/unit/test_deps_utils.py b/tests/unit/test_deps_utils.py index 0b24a739e..1933f6cd2 100644 --- a/tests/unit/test_deps_utils.py +++ b/tests/unit/test_deps_utils.py @@ -62,6 +62,15 @@ def test_direct_child_of_package(self, tmp_path): child.mkdir(parents=True) assert _is_nested_under_package(child, modules) is True + def test_child_of_manifestless_package(self, tmp_path): + """A subdirectory under a package with only .apm is nested.""" + modules = tmp_path / "apm_modules" + pkg = modules / "org" / "repo" + _make_apm_dir(pkg) + child = pkg / "sub" / "deep" + child.mkdir(parents=True) + assert _is_nested_under_package(child, modules) is True + def test_not_nested_when_no_parent_yml(self, tmp_path): """Candidate directly under apm_modules is not nested.""" modules = tmp_path / "apm_modules" @@ -480,6 +489,18 @@ def test_three_level_ado_packages(self, tmp_path): result = _scan_installed_packages(tmp_path) assert "org/project/repo" in result + def test_nested_package_manifest_is_part_of_parent(self, tmp_path): + """A package manifest nested inside an installed package is not top-level.""" + parent = tmp_path / "_local" / "package" + nested = parent / "sub-package" + nested.mkdir(parents=True) + _make_apm_yml(parent, "package") + _make_apm_yml(nested, "sub-package") + + result = _scan_installed_packages(tmp_path) + + assert result == ["_local/package"] + def test_hidden_dirs_skipped(self, tmp_path): """Directories starting with '.' are skipped.""" hidden = tmp_path / "org" / ".hidden"