fix(deps): preserve nested package manifests during prune

[danielmeppiel]

fix(deps): preserve nested package manifests during prune

TL;DR

Installed packages can contain nested apm.yml files that belong to the parent package. This change reuses the existing package-ancestry guard across dependency scans so nested manifests are not reported as top-level orphans and cannot be pruned independently.

Important

Closes #2067.

Problem (WHY)

• _scan_installed_packages() returned both _local/package and _local/package/sub-package, even though the consumer lockfile contained only the parent (reported reproduction).
• apm prune consumed that scan result and deleted the nested source directory as an orphan (reported prune output).
• apm deps list applied the nested-package guard only to SKILL.md-only directories, so a nested apm.yml remained visible as an orphan (reported root cause).

The destructive and advisory paths must classify the same on-disk package tree consistently. The issue's expected contract is: "A directory that lives inside an already-installed package is part of that package, not a separate top-level dependency."

Approach (WHAT)

#	Fix	Principle
1	Apply _is_nested_under_package() in the shared installed-package scanner.	"Favor small, chainable primitives over monolithic frameworks."
2	Apply the same guard without manifest-type gating in dependency list and tree scans.	"Grounding outputs in deterministic tool execution transforms probabilistic generation into verifiable action."
3	Add regression traps at the scanner and CLI classification boundaries.	"agents pattern-match well against concrete structures"

Implementation (HOW)

• src/apm_cli/commands/deps/_utils.py - filters candidates whose ancestor already has apm.yml, using the existing ancestry helper. The scanner still supports GitHub, ADO, and standalone subdirectory package layouts.
• src/apm_cli/commands/deps/cli.py - removes the SKILL.md-only restriction from both manual filesystem scans so nested package manifests and nested skill manifests follow one ownership rule.
• tests/unit/test_deps_utils.py - proves the shared scanner returns only _local/package when a nested subpackage also has apm.yml.
• tests/unit/commands/test_deps_cli_helpers.py - proves dependency listing omits the nested candidate from both installed and orphan classifications.

No user-facing command, flag, manifest schema, or documented contract changed, so no documentation or README update is included.

Diagrams

Legend: the dashed stage is the reused guard that now protects every filesystem scan before orphan classification.

flowchart LR
    subgraph Scan[Filesystem scan]
        S1["Find apm.yml or package marker"]
    end
    subgraph Classify[Package ownership]
        C1["Check ancestor apm.yml"]
        C2["Keep top-level package"]
        C3["Skip nested package"]
    end
    subgraph Consume[Consumers]
        D1["apm deps list"]
        D2["apm deps tree"]
        P1["apm prune"]
    end
    S1 --> C1
    C1 -->|"no package ancestor"| C2
    C1 -->|"package ancestor exists"| C3
    C2 --> D1
    C2 --> D2
    C2 --> P1
    classDef new stroke-dasharray: 5 5;
    class C1 new;

Loading

Trade-offs

• Reuse the existing ancestry guard instead of introducing scanner depth rules. Depth-based filtering would break supported GitHub, ADO, and subdirectory layouts; ancestry expresses package ownership directly.
• Filter before orphan classification instead of exempting nested paths during deletion. A deletion-only exception would leave apm deps list and apm prune inconsistent.
• Keep scope to package discovery. Lockfile schema, install layout, and audit policy remain unchanged because the false orphan originates in filesystem scanning.

Benefits

1. A parent plus nested apm.yml produces one installed-package entry instead of two.
2. apm deps list no longer reports the nested subpackage as a direct orphan.
3. apm prune no longer receives the nested subpackage as a deletion candidate.
4. The two new regression tests fail when the ancestry guards are disabled.

Validation

Relevant unit and integration suites:

........................................................................ [ 77%]
.....................                                                    [100%]
93 passed in 4.53s

Full CI lint mirror

All checks passed!
1410 files already formatted
YAML, file-length, and relative_to guards passed

--------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00)

[*] Rule A: get_bearer_provider boundary (any reference)
[*] Rule B: git ls-remote auth-delegated annotation
[+] auth-signal lint clean

Mutation-break proof:

FAILED tests/unit/test_deps_utils.py::TestScanInstalledPackages::test_nested_package_manifest_is_part_of_parent
FAILED tests/unit/commands/test_deps_cli_helpers.py::TestResolveScopeDepsWithPackages::test_nested_package_manifest_is_not_listed_independently
2 failed in 0.60s
Mutation killed: regression tests failed as expected (pytest exit 1)

Scenario Evidence

#	Scenario (user promise)	Principle(s)	Test(s) proving it	Type
1	A nested subpackage inside an installed parent is not treated as a top-level installed dependency.	Governed by policy, DevX (pragmatic as npm)	tests/unit/test_deps_utils.py::TestScanInstalledPackages::test_nested_package_manifest_is_part_of_parent (regression-trap for #2067)	unit
2	apm deps list does not report the nested subpackage as an orphan.	Governed by policy, DevX (pragmatic as npm)	tests/unit/commands/test_deps_cli_helpers.py::TestResolveScopeDepsWithPackages::test_nested_package_manifest_is_not_listed_independently (regression-trap for #2067)	unit
3	Existing virtual-package orphan detection remains consistent across supported layouts.	Governed by policy, Portability by manifest	tests/integration/test_virtual_package_orphan_detection.py	integration

How to test

• Create apm_modules/_local/package/apm.yml and apm_modules/_local/package/sub-package/apm.yml.
• Run apm deps list and confirm only _local/package is listed.
• Run apm prune --dry-run and confirm _local/package/sub-package is not a deletion candidate.
• Run the relevant pytest command from Validation and confirm all 93 tests pass.
• Run the CI lint mirror and confirm every gate exits zero.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

[Copilot]

Pull request overview

Fixes a destructive edge case in dependency discovery where an installed package containing a nested apm.yml could be misclassified as a separate top-level package, causing apm deps list to report a false orphan and apm prune to delete the nested directory.

Changes:

• Apply the existing _is_nested_under_package() ancestry guard inside the shared installed-package scanner so nested manifests are excluded from top-level results.
• Remove the previous SKILL.md-only gating so both apm.yml and SKILL.md nested manifests are consistently treated as owned by their parent package in deps list and deps tree.
• Add regression tests covering nested-manifest behavior at the scanner layer and the CLI helper classification layer.

Show a summary per file

File	Description
src/apm_cli/commands/deps/_utils.py	Filters installed-package scan results to exclude candidates nested under an existing package root.
src/apm_cli/commands/deps/cli.py	Aligns filesystem candidate scanning for deps list/tree to consistently skip nested manifests.
tests/unit/test_deps_utils.py	Adds unit coverage ensuring the shared scanner only returns the parent when a nested apm.yml exists.
tests/unit/commands/test_deps_cli_helpers.py	Adds unit coverage ensuring deps-list classification omits nested manifests as independent installed/orphan entries.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Low

[danielmeppiel]

APM Review Panel: ship_now

Fixes nested-package ownership semantics so scanner, deps, and prune consistently treat nested package content as parent-owned, closing #2067 with mutation-validated integration coverage.

cc @danielmeppiel @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

All panel personas converge on ship_now. The fix is surgically scoped: one shared ancestry predicate now protects installed-package scanning, dependency listing/tree display, and prune. The final integration test runs install, update, preservation prune, then parent removal; it proves the complete parent directory is removed while unrelated package material survives. Security, auth, CLI output, and performance reviewers found no actionable concerns.

Aligned with: Portable by manifest: manifest-declared parents own nested package content across CLI verbs; Secure by default: safe_rmtree containment is unchanged; Pragmatic as npm: prune keeps declared package content and removes the whole package only after its declaration is removed.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	0	2	Shared predicate is the simplest correct design; current bounded scan cost is acceptable.
CLI Logging Expert	0	0	0	No CLI output surface changed.
DevX UX Expert	0	0	0	No validated findings; legacy response shape failed schema validation twice.
Supply Chain Security Expert	0	0	0	Package boundaries tighten; deletion containment is unchanged.
OSS Growth Hacker	0	0	0	Required user-facing CHANGELOG entry is present.
Doc Writer	0	0	0	CHANGELOG and helper documentation are accurate.
Test Coverage Expert	0	0	0	Exact-HEAD lifecycle integration test passed.
Performance Expert	0	0	0	Bounded O(N*D) filesystem scan is acceptable.

B = highest-severity signals, R = recommendations, N = nits. Counts are advisory signals.

Architecture

flowchart TD
    A["install / update / prune / deps tree"] --> B["scan apm_modules"]
    B --> C{"candidate has apm.yml or .apm?"}
    C -- No --> D["skip"]
    C -- Yes --> E{"ancestor has apm.yml or .apm?"}
    E -- Yes --> F["skip: owned by parent"]
    E -- No --> G["classify top-level package"]
    G --> H{"still declared?"}
    H -- Yes --> I["keep complete package"]
    H -- No --> J["remove complete package directory"]

Loading

Folded in this run

• (Copilot) Renamed the CLI helper test so its name matches the asserted classification -- cec792e57.
• (Panel) Added an empirical install/update/prune lifecycle test with unrelated-material preservation -- cec792e57.
• (Panel) Aligned .apm-only parent detection and added direct dependency-tree coverage -- 538527d63.
• (Panel) Added the required CHANGELOG entry and corrected the package-boundary docstring -- f9a6fba37.

Regression-trap evidence

• tests/integration/test_intra_package_cleanup.py::TestNestedPackagePrune::test_prune_preserves_nested_content_then_removes_whole_parent -- deleted the _scan_installed_packages ancestry guard; test failed because nested content was removed; guard restored.
• tests/unit/test_deps_utils.py::TestIsNestedUnderPackage::test_child_of_manifestless_package -- deleted the .apm parent condition; test failed; guard restored.
• tests/unit/commands/test_deps_cli_phase3w4.py::TestBuildDepTree::test_nested_package_manifest_skipped -- deleted the tree ancestry guard; test failed because the nested package appeared independently; guard restored.

Validation

• Exact head: f9a6fba375ca64b322c7984a874a3179db47918a.
• Relevant suite: 92 passed before the final documentation fold; 89 passed after it.
• Full lint mirror: ruff check and format silent; YAML I/O, file length, relative path, pylint R0801, and auth-signal guards passed.
• Exact-head CI: all 13 checks passed, including both test shards, coverage combine, lint, CodeQL, merge workflow, binary smoke, spec conformance, self-check, NOTICE, and CLA. Run: https://github.com/microsoft/apm/actions/runs/29062607630

Recommendation

Ready for maintainer review. No recommended follow-ups remain.

---

Full per-persona findings

Python Architect

• [nit] The ancestry walk is O(N*D), but package depth is bounded and measured impact is negligible.
• [nit] No additional design pattern is warranted; the shared predicate is the simplest correct shape.

All other active personas returned no findings. Auth Expert was inactive because the diff is authentication-neutral.

_{This panel is advisory; the maintainer makes the shipping decision.}

[danielmeppiel]

APM Review Panel: ship_with_followups

Restores npm-like nested dependency semantics in prune, closing #2067 with deterministic 8-level E2E proof and three mutation probes.

cc @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

Panel convergence is strong: eight active panelists affirm the fix is sound, and the test evidence is decisive. Two independent exact-target E2E runs passed at commit 848d42c, exercising install/list/tree/prune through eight nested levels. A prior contrary test claim came from the wrong worktree and is discarded. Three mutation probes failed as designed and passed after restoration, confirming the ownership guard, full tree traversal, and canonical local-path match are load-bearing.

The substantive concern is Python call-stack exhaustion on a crafted, extremely deep lockfile. A hard depth cap would violate the explicit full-graph contract. Iterative explicit-stack traversal resolves the concern without truncating legitimate graphs. The prune documentation also defines orphan status too narrowly and should mention lockfile-resolved transitives.

Dissent. Supply Chain Security suggested a hard cap, while the parent requirement forbids one. Python Architecture's iterative traversal resolves the same risk without sacrificing full depth.

Aligned with: Portable by manifest: every lockfile node remains visible. Secure by default: parent-owned content survives prune. Pragmatic as npm: inventory follows the resolved graph, not arbitrary source manifests.

Growth signal. The pain-first release note makes this package-manager trust fix easy for adopters to recognize.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	1	2	Cycle-guarded traversal and ownership filtering are sound; remove call-stack limits iteratively.
CLI Logging Expert	0	0	2	Output is consistent; a cycle marker would improve diagnostics.
DevX UX Expert	0	1	1	The npm-like mental model is restored; one pre-existing recovery message remains vague.
Supply Chain Security Expert	0	1	1	Prune safety is stronger; iterative traversal avoids deep-lockfile stack exhaustion.
OSS Growth Hacker	0	0	2	Docs and changelog reinforce trust; lead with user pain.
Doc Writer	0	1	0	Full-depth docs are accurate; prune's orphan definition needs the lock graph.
Test Coverage Expert	0	0	1	Critical behavior is defended by E2E and mutation evidence.
Performance Expert	0	0	1	Typical graphs are fast; an iterative walk avoids per-frame overhead.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 5 follow-ups

1. [Python Architect] Convert Rich and fallback tree rendering to iterative explicit-stack traversal -- eliminates Python recursion limits without introducing a depth cap.
2. [Doc Writer] Define prune orphans against direct declarations and the lockfile-resolved graph -- current prose omits retained transitives.
3. [CLI Logging Expert] Mark repeated ancestors as circular -- makes intentional cycle suppression visible.
4. [DevX UX Expert] Improve the pre-existing unresolved-parent guidance -- gives users a concrete recovery path.
5. [Test Coverage Expert] Strengthen no-Rich traversal coverage -- protects fallback parity.

Architecture

classDiagram
    direction LR
    class ScanInstalledPackages
    class NestedPackageGuard
    class BuildDependencyTree
    class RichTreeRenderer
    class TextTreeRenderer
    class LockfileDependency
    ScanInstalledPackages ..> NestedPackageGuard : filters
    BuildDependencyTree ..> NestedPackageGuard : filters
    BuildDependencyTree ..> LockfileDependency : reads
    BuildDependencyTree ..> RichTreeRenderer : renders
    BuildDependencyTree ..> TextTreeRenderer : renders

Loading

flowchart TD
    CLI[apm deps list / tree / prune] --> Scan[scan apm_modules]
    Scan --> Ownership{nested under package?}
    Ownership -->|yes| Ignore[parent-owned content]
    Ownership -->|no| Match[match canonical lock path]
    Match --> Graph[resolved_by dependency graph]
    Graph --> Cycle{ancestor repeated?}
    Cycle -->|yes| Stop[stop this path]
    Cycle -->|no| Render[render child and continue]

Loading

Recommendation

The exact-target regression and mutation evidence prove the user promise. Fold iterative no-cap traversal and the prune-doc correction in this PR; the remaining diagnostic and fallback items are small quality improvements on the same surface.

---

Full per-persona findings

Python Architect

• [recommended] Recursive traversal can hit Python's recursion limit on pathological graphs at src/apm_cli/commands/deps/cli.py:73.
  Convert to iterative DFS with an explicit stack; do not add a hard depth cap.
• [nit] Rich and text rendering duplicate traversal logic.
• [nit] The optional ancestor sentinel is less explicit than a dedicated walk abstraction.

CLI Logging Expert

• [nit] Repeated ancestors are silently skipped at src/apm_cli/commands/deps/cli.py:85.
• [nit] The text fallback uses click.echo; this is pre-existing and appropriate for structured output.

DevX UX Expert

• [recommended] The pre-existing (could not determine parent) annotation lacks actionable recovery guidance.
• [nit] Keep command-guide notes within its scannable table structure.

Supply Chain Security Expert

• [recommended] Recursive traversal permits stack exhaustion from a crafted very deep lockfile at src/apm_cli/commands/deps/cli.py:89.
  Use an iterative stack rather than the suggested hard cap.
• [nit] Symlink resolution could strengthen ancestry checks, while the deletion chokepoint already enforces containment.

OSS Growth Hacker

• [nit] Lead the CHANGELOG entry with the phantom-orphan/prune pain.
• [nit] Contributor attribution can reinforce community norms when applicable.

Auth Expert -- inactive

Dependency inventory, tree, and prune behavior do not change authentication, token, credential, or host-selection surfaces.

Doc Writer

• [recommended] Prune docs omit lockfile-resolved transitives from the orphan definition at docs/src/content/docs/reference/cli/prune.md:28.
  Proof (passed): tests/integration/test_transitive_chain_e2e.py::test_deps_commands_follow_full_lock_graph_and_ignore_embedded_manifests -- exact target run passed in 3.73s.

Test Coverage Expert

• [nit] The no-Rich fallback is covered below the full E2E tier.
  Proof (passed): exact 8-level CLI E2E passed in 3.06s; unit fallback coverage also passes.

Performance Expert

• [nit] Immutable ancestor-set copies add minor allocation cost at src/apm_cli/commands/deps/cli.py:94; an iterative traversal can avoid call-stack growth.

_{This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.}

[danielmeppiel]

APM Review Panel: ship_with_followups

Iterative dependency-tree walking now provides full-depth rendering, cycle detection, and Rich/plain-text parity; only narrow polish remains.

cc @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

Eight active panelists found no correctness, security, documentation, test, or performance concern. The iteration-one recommendations are folded and verified: 81 focused tests pass, the actual CLI E2E remains green, and all six mutation probes fail under mutation and pass after restoration.

The remaining signals converge on small in-scope cleanup. _add_tree_children is Rich-only, so its has_rich argument is a false affordance with a dead branch. The command help can also name full resolved-graph behavior directly. Exact connector characters are implementation detail, but one direct text-walker assertion would make fallback parity explicit.

Aligned with: Portable by manifest: rendering follows apm.lock.yaml. Pragmatic as npm: the complete graph is visible without silent truncation. OSS community driven: direct help and recovery text simplify diagnosis.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	0	2	Architecture is sound; remove one vestigial parameter.
CLI Logging Expert	0	0	1	Rich and text output agree; the same parameter is dead.
DevX UX Expert	0	0	1	Tighten command help copy.
Supply Chain Security Expert	0	0	0	No current security concern.
OSS Growth Hacker	0	0	0	User-facing communication is clear.
Doc Writer	0	0	0	Changed docs are accurate and complete.
Test Coverage Expert	0	0	1	Critical contracts are covered; exact text prefixes could be explicit.
Performance Expert	0	0	0	Traversal cost is appropriate for CLI display.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 3 follow-ups

1. [Python Architect] Remove _add_tree_children(..., has_rich) -- the function is Rich-only and the false branch is unreachable.
2. [DevX UX Expert] Tighten list/tree help text -- describe manifest-plus-lock inventory and complete resolved-tree rendering.
3. [Test Coverage Expert] Add one exact ASCII text-walker prefix assertion -- make fallback shape explicit without coupling E2E to Rich glyphs.

Architecture

flowchart TD
    CLI[apm deps tree] --> Build[build direct and resolved_by indexes]
    Build --> Walk[iterative depth-first walk]
    Walk --> Cycle{ancestor repeated?}
    Cycle -->|yes| Mark[render circular marker]
    Cycle -->|no| Push[push children]
    Mark --> Rich[Rich renderer]
    Mark --> Text[text renderer]
    Push --> Rich
    Push --> Text

Loading

Recommendation

Fold the dead-parameter cleanup, help copy, and small text-prefix test. These are narrow changes on the current surface; the substantive behavior is already proven.

---

Full per-persona findings

Python Architect

• [nit] _add_tree_children accepts has_rich, but all production callers are inside the Rich branch; remove the false affordance.
• [nit] The branch map retains completed entries; measured scale makes this non-actionable.

CLI Logging Expert

• [nit] Remove the dead has_rich branch so the Rich-only contract is explicit.

DevX UX Expert

• [nit] List/tree --help copy under-describes manifest-plus-lock and all-depth behavior.

Supply Chain Security Expert

No findings.

OSS Growth Hacker

No findings.

Auth Expert -- inactive

No authentication, credential, token, or host-classification surface changed.

Doc Writer

No findings.

Test Coverage Expert

• [nit] The E2E proves increasing nesting depth, while exact text connectors are only covered below E2E tier. The user promise is covered.

Performance Expert

No findings.

_{This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.}

[danielmeppiel]

APM Review Panel: ship_with_followups

The lock-graph refactor passes 108 focused tests with all seven mutation probes killed; four small contract-alignment edits remain.

cc @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

The panel converges on implementation correctness. Exact-target tests are green, two wrong-worktree failure claims were disproved by rerunning the named tests, and security, auth, performance, and coverage lenses have no current concern.

Four narrow edits remain in scope. The docs should state categorically that manifests embedded inside installed package source are parent-owned. Orphan warnings should refer to the resolved dependency graph, not only apm.yml. List help should describe the user outcome, and the iterative walk should expose its LockedDependency element type.

Aligned with: Portable by manifest: the lock graph defines dependency identity. Secure by default: parent-owned source survives prune. Pragmatic as npm: inventory and diagnostics use familiar package-manager semantics.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	0	1	Implementation is sound; strengthen one element type.
CLI Logging Expert	0	0	1	Correct one stale orphan-warning parenthetical.
DevX UX Expert	0	1	0	Use user-intent list help wording.
Supply Chain Security Expert	0	0	0	No current concern.
OSS Growth Hacker	0	0	0	No in-PR communication concern.
Doc Writer	0	1	0	Remove an unsupported embedded-manifest exception.
Test Coverage Expert	0	0	0	Exact-target coverage is green.
Performance Expert	0	0	0	No material performance fault.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 4 follow-ups

1. [Doc Writer] Remove the lockfile exception for embedded manifests -- scanners categorically treat nested source manifests as parent-owned.
2. [CLI Logging Expert] Change (not in apm.yml) to resolved-graph wording -- expected paths include lockfile transitives.
3. [DevX UX Expert] Use List installed APM dependencies and their primitives -- describe user intent in help.
4. [Python Architect] Type tree children as list[LockedDependency] -- make the shared walker contract explicit.

Recommendation

Fold these four small edits, rerun the exact-target evidence, and perform the terminal advisory pass. The required behavior is already proven.

---

Full per-persona findings

Python Architect

• [nit] Replace bare child lists with list[LockedDependency] annotations.

CLI Logging Expert

• [nit] Orphan warnings say (not in apm.yml) although lockfile-resolved transitives are also expected.

DevX UX Expert

• [recommended] List help uses implementation wording rather than the user's goal.

Supply Chain Security Expert

No findings.

OSS Growth Hacker

No in-PR findings; release narrative is release-prep scope.

Auth Expert -- inactive

No authentication surface changed.

Doc Writer

• [recommended] Changed docs describe a lockfile exception for nested manifests that the scanner does not implement.

Test Coverage Expert

No findings; exact-target tests pass.

Performance Expert

No findings.

_{This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.}

[danielmeppiel]

APM Review Panel: ship_now

Full-depth, cycle-safe dependency diagnostics and parent-owned manifest protection are complete, with concise recovery UX and exact-target proof.

cc @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

All nine panel personas converge after four bounded iterations. Every in-scope finding is folded at 67299472a083e20af1646d581e5edf5e5dc4b105: iterative no-cap traversal, path-local cycle detection, Rich/text parity, resolved-graph orphan wording, user-focused help, recovery guidance, exact fallback nesting, typed lock nodes, and accurate ownership/prune docs.

The evidence is strong: 112 exact-target tests pass in 11.99s. The actual CLI E2E exercises install, list, tree, and prune across eight lockfile depths plus shallow and deep embedded manifests. A 1100-node unit chain proves there is no Python call-stack cutoff. Seven mutation probes fail under mutation and pass after production restoration. Wrong-worktree results were discarded and rerun in the target worktree.

Aligned with: Portable by manifest: inventory follows the complete resolved graph. Secure by default: parent-owned package content survives prune and cycles cannot exhaust the call stack. Pragmatic as npm: diagnostics show the real graph and include a direct recovery action.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	0	0	Shared iterative traversal is typed, cycle-safe, and unbounded by call depth.
CLI Logging Expert	0	0	0	Rich and text output are consistent and actionable.
DevX UX Expert	0	0	0	Help, recovery, inventory, and prune match the package-manager mental model.
Supply Chain Security Expert	0	0	0	No new deletion, credential, or integrity risk.
OSS Growth Hacker	0	0	0	Changed docs and changelog communicate the safety fix clearly.
Doc Writer	0	0	0	Dependency and prune references match runtime behavior.
Test Coverage Expert	0	0	0	Critical contracts have E2E, unit, and mutation regression traps.
Performance Expert	0	0	0	Iterative display traversal has no material performance fault.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Architecture

classDiagram
    class LockedDependency
    class TreeWalk
    class RichRenderer
    class TextRenderer
    TreeWalk ..> LockedDependency : traverses
    RichRenderer ..> TreeWalk : consumes
    TextRenderer ..> TreeWalk : consumes

Loading

flowchart TD
    Lock[apm.lock.yaml] --> Index[resolved_by index]
    Index --> Walk[iterative depth-first walk]
    Walk --> Cycle{ancestor repeated?}
    Cycle -->|yes| Mark[render circular marker]
    Cycle -->|no| Continue[push children]
    Mark --> Output[Rich or text output]
    Continue --> Output

Loading

Recommendation

All panel signals converge on ship_now. No current findings, deferred items, or follow-ups remain; the exact-target implementation and evidence are ready for maintainer review.

---

Full per-persona findings

Python Architect

No findings.

CLI Logging Expert

No findings after the final wording fold.

DevX UX Expert

No findings after the final help and recovery fold.

Supply Chain Security Expert

No findings.

OSS Growth Hacker

No in-PR findings.

Auth Expert -- inactive

No authentication, token, credential, or host-classification surface changed.

Doc Writer

No findings.

Test Coverage Expert

No findings; 112 exact-target tests pass and seven mutations were killed.

Performance Expert

No findings.

_{This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.}

Reservations carried from strategic alignment

• Reassess nested-package visibility after fix(deps): match local transitive dependencies by install path #2099 at arbitrary graph depth: addressed by the 8-node actual CLI lock-chain E2E, categorical parent-owned manifest filtering, and canonical local install-path assertions.

Folded in this run

• Iterative no-cap tree traversal and visible cycle markers (2609eb7e1).
• Resolved-graph prune definition, pain-first changelog, and recovery guidance (2609eb7e1).
• Rich-only renderer cleanup and exact ASCII fallback nesting (8b61f259f).
• Parent-owned manifest docs, resolved-graph orphan wording, and typed lock nodes (69609ad15).
• Concise tree help and unresolved-node recovery text (67299472a).

Copilot signals reviewed

• Two fetch rounds found no Copilot inline comments. The summary-only review introduced no actionable signal.

Validation

• Final head: 6944af6b3cd505b6b1b19c0360f3a1301e1aad2d
• Local lint: all seven CI-mirror gates passed (ruff check, format check, YAML I/O, 2100-line limit, portable relpaths, pylint R0801, auth signals).
• Regression suite: 112 focused tests passed; seven production mutations were killed and restored.
• Exact-SHA CI: all required checks completed successfully, including Lint, both Linux test shards, Coverage Combine, PR Binary Smoke, APM Self-Check, CodeQL, Merge Gate, spec conformance, docs build, NOTICE, and CLA.

PR	Head	Panel stance	Iterations	Folds	Deferrals	Copilot rounds	CI	Mergeable	Merge state	Notes
#2092	6944af6	ship_now	4	10	0	2	green	MERGEABLE	BLOCKED	pending required review

[danielmeppiel]

APM Review Panel: ship_now

Fixes a data-loss bug where apm prune deleted parent-owned nested package content, and lifts deps tree to full-depth cycle-aware rendering -- pure trust and visibility table-stakes.

cc @danielmeppiel @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

This PR eliminates a class of silent data loss: nested apm.yml files embedded inside an installed dependency were misclassified as independent orphans and pruned. The fix reuses the existing _is_nested_under_package ancestry guard at all three filesystem scan sites (installed-package scanner, deps list, deps tree), ensuring a single ownership rule governs classification everywhere. The secondary gain -- replacing the hard depth-5 cap with an iterative frozenset-based cycle-detecting walker that marks cycles with a (circular) suffix -- means apm deps tree now renders arbitrarily deep dependency graphs accurately. Together these changes deliver on two user promises: "your package manager never eats your code" and "you can see your real dependency graph."

All nine panel seats converge on ship. Supply-chain-security confirms no regression in prune's conservative classification. Python-architect validates the ancestry-guard abstraction and the iterative walker's soundness. Performance-expert confirms O(N*D) cost is bounded and acceptable on realistic graphs (sub-millisecond at production depths). Test-coverage-expert confirms all four critical user promises carry regression traps at both unit and integration tiers, with a seven-gate mutation-break proof green on the final head. The three recommended findings (doc-writer CHANGELOG placement + merge duplication, devx prune --help drift) were folded into the final head before this pass; no recommended or blocking items remain open.

The remaining nits are strictly deferrable: orphan-message wording preferences, circular-marker styling, and optional NamedTuple graduation. None affect correctness, security, or user-facing contracts on this commit.

Dissent. Mild tension between cli-logging-expert/oss-growth-hacker (prefer orphan parenthetical to name apm.yml for unfamiliar users) and devx-ux-expert (aligned the wording to "resolved dependency graph" for consistency with prune.md); both sides agree this is a post-merge polish nit, not a blocking concern.

Aligned with: Governed by policy -- orphan classification now correctly implements the ownership invariant: a directory inside an installed package belongs to that package, period. Pragmatic as npm -- mirrors the npm prune mental model where nested packages inside a parent are never treated as independent top-level packages. Portable by manifest -- preserves nested apm.yml files that define sub-package contracts. OSS community-driven -- directly fixes community-reported issue #2067 with regression traps that protect the contract going forward.

Growth signal. The data-loss fix is pure trust table-stakes for the next release announcement: "your package manager never eats your code." The deps-tree full-depth rendering is a secondary beat aimed at the monorepo and enterprise audience -- "shows your real graph, no depth limits, with cycle detection." Together they earn a "reliability and visibility" section in release notes and reinforce the narrative that APM handles production-scale dependency graphs.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	0	1	Ancestry guard reuse is the correct abstraction; iterative walker with frozenset cycle detection is sound and eliminates the depth-5 cap.
CLI Logging Expert	0	0	2	CLI output clean: ASCII-only tree prefixes, consistent circular/orphan messaging across Rich and non-Rich, proper CommandLogger routing.
DevX UX Expert	0	1	2	Nested-manifest ownership fix aligns with npm/pip mental models; prune --help drift folded on final head.
Supply Chain Security Expert	0	0	0	Ancestry guard and cycle detection are sound; prune classification errs conservative; no security regression.
OSS Growth Hacker	0	0	2	CHANGELOG entry ships the trust narrative; tree-depth lift is a quiet "we handle real graphs" signal.
Performance Expert	0	0	3	Iterative tree walk and per-candidate ancestry guard are both O(N*D) on realistic lock graphs; no user-visible regression.
Doc Writer	0	2	0	Docs prose accurate; CHANGELOG placement + merge-duplication findings both folded on final head.
Test Coverage Expert	0	0	2	All four critical user promises carry regression traps at unit and integration/E2E tiers.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 4 follow-ups

1. [CLI Logging Expert] Orphan parenthetical could name apm.yml alongside "resolved dependency graph" for users unfamiliar with the term. -- Reduces cognitive load for first-time users encountering an orphan warning; low-cost copy change that does not affect correctness.
2. [CLI Logging Expert] Circular marker inherits [dim] styling; a [yellow](circular)[/yellow] infix would make cycle-stop nodes visually pop in Rich output. -- Improves scanability of deep trees with cycles; purely cosmetic, zero correctness impact.
3. [Python Architect] Optional NamedTuple graduation for the 4-tuple yielded by _walk_tree_children if a third consumer appears. -- Improves readability and IDE support for tuple unpacking; defer until a third call-site materializes.
4. [Performance Expert] O(N*D) frozenset-union and tuple-key hashing costs are acceptable at production depths (<0.1ms at D=20); revisit only if lock graphs exceed hundreds of nodes. -- Informational note for future scaling; no action needed on current workloads.

Architecture

classDiagram
    direction TB
    class DepsUtils {
        <<Module>>
        +_scan_installed_packages(dir) list
        +_is_nested_under_package(candidate, root) bool
        +_count_primitives(path) dict
        +_get_package_display_info(path) dict
    }
    class DepsCli {
        <<Module>>
        +_walk_tree_children(key, map) Iterator
        +_add_tree_children(branch, key, map) None
        +_echo_tree_children(key, map, prefix) None
        +_resolve_scope_deps(dir, logger) tuple
        +_build_dep_tree(dir) dict
    }
    class LockedDependency {
        <<ValueObject>>
        +repo_url str
        +depth int
        +resolved_by str
        +get_unique_key() str
    }
    class APMPackage {
        <<ValueObject>>
        +name str
        +version str
        +get_apm_dependencies() list
    }
    class CommandLogger {
        <<Lifecycle>>
        +warning(msg) None
        +info(msg) None
        +error(msg) None
    }
    class PruneCommand {
        <<Consumer>>
        +prune(dry_run) None
    }
    class HelpersModule {
        <<Module>>
        +_check_orphaned_packages() list
        +_expand_with_ancestors() set
    }
    DepsCli ..> DepsUtils : imports _is_nested_under_package
    DepsCli ..> LockedDependency : traverses
    DepsCli ..> APMPackage : reads manifest
    DepsCli ..> CommandLogger : logs output
    DepsUtils ..> APMPackage : reads manifest
    PruneCommand ..> DepsUtils : _scan_installed_packages
    PruneCommand ..> HelpersModule : _expand_with_ancestors
    HelpersModule ..> DepsUtils : _scan_installed_packages
    note for DepsUtils "Guard predicate: _is_nested_under_package walks up to root checking apm.yml or .apm"
    note for DepsCli "Iterator pattern: _walk_tree_children stack-based DFS with frozenset cycle guard"

Loading

flowchart TD
    CLI["apm deps list / tree / prune"]
    CLI --> SOURCE{"source?"}
    SOURCE -->|"deps tree + lockfile"| BUILD["_build_dep_tree parse lockfile deps by depth + resolved_by"]
    SOURCE -->|"deps list or no lockfile"| RGLOB["[FS] rglob apm_modules/*"]
    BUILD --> WALK["_walk_tree_children stack-based DFS, frozenset ancestors"]
    WALK --> CYCLECHECK{"child_key in ancestors?"}
    CYCLECHECK -->|"yes"| CIRCULAR["yield circular=True skip children push"]
    CYCLECHECK -->|"no"| NOTCIRC["yield circular=False push grandchildren onto stack"]
    NOTCIRC --> WALK
    CIRCULAR --> RENDERTREE["_add_tree_children / _echo_tree_children append circular suffix"]
    NOTCIRC --> RENDERTREE
    RGLOB --> NESTED["[FS] _is_nested_under_package walk parent dirs for apm.yml or .apm"]
    NESTED -->|"nested"| SKIP["skip: parent-owned content"]
    NESTED -->|"top-level"| CANDIDATES["scanned_candidates"]
    CANDIDATES --> ORPHAN["orphan = key not in declared_with_ancestors"]
    ORPHAN --> RENDERLIST["render table + orphan warnings"]
    BUILD --> NESTED2["[FS] _is_nested_under_package fallback scan when no lockfile"]
    NESTED2 -->|"nested"| SKIP2["skip nested"]
    NESTED2 -->|"top-level"| SCAN["scanned_packages for tree fallback"]

Loading

Recommendation

Merge to main and include in the next release with a "reliability and visibility" section in the announcement. The single highest-signal follow-up to track post-merge is the orphan-message wording nit (cli-logging-expert): adding "apm.yml" to the parenthetical so unfamiliar users know exactly which file governs ownership. File as a low-priority issue and pick up in the next polish pass.

---

Full per-persona findings

Python Architect

• [nit] Design patterns assessment; optional NamedTuple graduation for the 4-tuple yielded by _walk_tree_children if a third consumer appears at src/apm_cli/commands/deps/cli.py:75
  Iterator/Generator (traversal separated from rendering) and Guard/Predicate (_is_nested_under_package applied uniformly across three scan sites) are the correct patterns at this scope. The bare 4-tuple is appropriate at two consumers with clean destructuring.

CLI Logging Expert

• [nit] Circular marker inherits [dim] styling in Rich tree at src/apm_cli/commands/deps/cli.py:124
  In a large tree every transitive node is dim, so the cycle-stop marker blends in. A [yellow](circular)[/yellow] infix inside the [dim] block would let the cycle annotation pop without breaking the dim convention. Purely cosmetic.
• [nit] Orphan parenthetical could name apm.yml alongside the graph for unfamiliar users at src/apm_cli/commands/deps/cli.py:399
  "not in resolved dependency graph" is accurate but may puzzle a user who does not know what the resolved dependency graph is; naming the concrete artifacts (apm.yml, lockfile) is a friendlier middle ground.

DevX UX Expert

• [recommended] prune --help said "not listed in apm.yml" while deps list says "not in resolved dependency graph" -- FOLDED on final head at src/apm_cli/commands/prune.py:24
  The --help surface is the primary discoverability mechanism and should match the docs this PR updated. Resolved on the final head by aligning the Click help= and docstring to the prune.md resolved-graph orphan definition (help text only; no logic change).
• [nit] deps list help could mention nested-manifest filtering for discoverability at src/apm_cli/commands/deps/cli.py:454
  A user hitting the old bug might search --help for "nested" or "orphan"; a brief note would make the fix discoverable from the CLI itself. Minor since behavior is now correct by default.
• [nit] Positive signal: "could not place in tree; run apm install to resolve" names the recovery action (Failure-mode-is-the-product); no change needed.

Supply Chain Security Expert

No findings.

OSS Growth Hacker

• [nit] CHANGELOG bullet packed a data-loss fix and a tree-rendering improvement into one sentence -- FOLDED (split into two bullets on final head) at CHANGELOG.md:50
  Users scanning for "did prune delete my stuff?" locate the fix faster if it stands alone; a separate bullet for the deps-tree depth + cycle-marking upgrade also lets release-note writers craft two story beats.
• [nit] Orphan warning changed to graph jargon at src/apm_cli/commands/deps/cli.py:255
  "not declared in apm.yml or reached transitively" keeps the actionable pointer to the manifest for first-time users.

Performance Expert

• [nit] _walk_tree_children carries O(D^2) per-node cost from frozenset union and tuple unpacking on deep chains at src/apm_cli/commands/deps/cli.py:101
  Noise in production (<0.1ms at D=20). If profiling ever shows this as a hot spot, switch the per-node frozenset to a mutable set with add/discard around the stack loop and use a mutable list for the path. No action needed now.
• [nit] branches dict keys on growing tuples -- O(N*D) hashing, acceptable at src/apm_cli/commands/deps/cli.py:120
  At N=100, D=10 that is ~1000 hash operations (microseconds). A flat integer id would be cheaper only if the tree grows to thousands of nodes.
• [nit] _is_nested_under_package issues 2 stat() per ancestor level for each candidate -- O(N*D) but bounded by realistic layouts
  Typical N<50 at depths 2-4 = ~200-400 stat() calls per scan (~0.2-2ms on warm VFS cache). No regression; the guard now applies to all matching candidates, widening the count slightly but still noise.

Doc Writer

• [recommended] fix(deps): preserve nested package manifests during prune #2092 changelog entry sat under released [0.24.1] not [Unreleased] -- FOLDED at CHANGELOG.md:50
  Keep-a-Changelog freezes dated release sections; unreleased changes belong under [Unreleased]. Resolved on the final head by relocating the entry (split into the data-loss/orphan fix and the deps-tree full-depth + (circular) beat).
• [recommended] Merge duplicated fix(install): actionable hint when self-hosted GitLab host swallows subpath #2074 and fix: retain dev dependencies (supersedes #2042, closes #2033) #2102 entries inside [0.24.1] -- FOLDED at CHANGELOG.md:53
  Single-source changelog hygiene: the same fix must appear once. Resolved on the final head by dropping the merge-added duplicates so [0.24.1] again matches main byte-for-byte.

Test Coverage Expert

• [nit] PR-body Scenario Evidence row 2 + mutation block cited a stale test name -- FOLDED at tests/unit/commands/test_deps_cli_helpers.py:163
  Cited test_nested_package_manifest_is_not_an_orphan; actual is test_nested_package_manifest_is_not_listed_independently. Corrected in both places in the PR body.
• [nit] test_transitive_chain_e2e.py lacked module-level requires_apm_binary pytestmark -- FOLDED at tests/integration/test_transitive_chain_e2e.py:1
  Added pytestmark = pytest.mark.requires_apm_binary, matching test_intra_package_cleanup.py and the tests.instructions.md convention.

Auth Expert -- inactive

PR touches deps scan/prune logic, reference docs, CHANGELOG, and unit/integration tests; no authentication, token management, credential resolution, AuthResolver, host classification, or remote-host fallback surface is modified.

_{This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.}