Don't flag parent dirs of subdirectory packages as orphaned

[tillig]

Description

Fix false-positive orphan detection when dict-form dependencies (git: + path:) point at nested subdirectory packages. When a dependency like .apm/skills/commit-conventions is installed, the intermediate owner/repo/ directory contains a .apm/ subdirectory as a side effect. The orphan scanner picks this up as an "installed package," but since it's not directly in the expected set (only the deeper path is), it gets falsely flagged as orphaned.

The fix adds an ancestor check: a scanned directory is not considered orphaned if any expected install path starts with it as a path prefix. Applied consistently across all three orphan-detection sites (apm compile, apm prune, apm deps list).

Fixes #1050

Type of change

• Bug fix
• New feature
• Documentation
• Maintenance / refactor

Testing

• Tested locally
• All existing tests pass
• Added tests for new functionality (if applicable)

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Fixes false-positive orphan detection when dict-form subdirectory dependencies create intermediate parent directories that were previously flagged as orphaned.

Changes:

• Add an “ancestor of expected install path” check to orphan detection in apm prune, apm deps list, and _check_orphaned_packages.
• Add unit tests covering subdirectory dependency parent directory handling and regression cases.
• Document the fix in the changelog.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/unit/test_deps_list_tree_info.py	Adds a CLI-level regression test ensuring deps list output doesn’t report the parent directory as orphaned.
tests/unit/test_command_helpers.py	Adds unit tests validating _check_orphaned_packages behavior for subdirectory ancestors and real orphans.
src/apm_cli/commands/prune.py	Updates prune orphan filtering to ignore directories that are ancestors of expected install paths.
src/apm_cli/commands/deps/cli.py	Updates deps list orphan detection to ignore ancestors of declared subdirectory install paths.
src/apm_cli/commands/_helpers.py	Updates _check_orphaned_packages to ignore installed paths that are ancestors of expected install paths.
CHANGELOG.md	Notes the bug fix for false orphan detection of parent directories.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[github-actions]

APM Review Panel Verdict: REJECT

One required finding blocks merge: ancestor expansion in _expand_with_ancestors can silently suppress a real orphaned package when a subdirectory dependency shares the same owner/repo root.

Required before merge (1 item)

• [devx-ux-expert] Ancestor expansion can silently suppress a real orphan when a sibling subdirectory package is declared at src/apm_cli/commands/_helpers.py
  • Why: If a user has a genuinely orphaned top-level package at owner/repo (with apm.yml or .apm, not in apm.yml) AND a declared subdirectory dependency at owner/repo/.apm/skills/foo, _expand_with_ancestors adds owner/repo to the expanded expected set, so the real orphan is never flagged by apm prune or apm deps list. Users coming from npm/pip/cargo expect prune to be conservative: it should never silently miss a real orphan. A false negative here means stale packages accumulate on disk with no warning, which is the opposite of the safety guarantee prune is meant to provide.
  • Suggested fix: Before adding owner/repo to the ancestor set, check whether _scan_installed_packages also returns owner/repo as a standalone installed package; if it does and it is not independently in the declared expected set, it should still be flagged. Alternatively, restrict ancestor expansion to paths that are not themselves returned by _scan_installed_packages (do not suppress a path that is a real installed package, only suppress paths that exist solely as filesystem intermediaries).

Nits (9 items, skip if you want)

• [python-architect] Return type annotation on _expand_with_ancestors is unparameterized at src/apm_cli/commands/_helpers.py:161
• [python-architect] Docstring example understates the set _expand_with_ancestors returns at src/apm_cli/commands/_helpers.py:163
• [python-architect] ADO 3-level paths can produce a false-negative in orphan detection at src/apm_cli/commands/_helpers.py:171
• [cli-logging-expert] Orphan-warning wording is inconsistent between prune and deps list at src/apm_cli/commands/prune.py
• [cli-logging-expert] deps/cli.py orphan block uses console.print directly instead of CommandLogger at src/apm_cli/commands/deps/cli.py
• [cli-logging-expert] Test assertion comment explains old behavior, not new intent at tests/unit/test_deps_list_tree_info.py
• [devx-ux-expert] CHANGELOG leads with apm compile, which has no mental-model equivalent in npm/pip/cargo -- reorder to: apm prune, apm deps list, and apm compile at CHANGELOG.md
• [supply-chain-security-expert] _expand_with_ancestors has no explicit traversal guard on its inputs -- add an early-exit skip/raise for any path p where '..' in p.split('/') at src/apm_cli/commands/_helpers.py
• [oss-growth-hacker] CHANGELOG entry is written for maintainers, not users -- open with the user-visible symptom at CHANGELOG.md

CEO arbitration

The panel reached consensus on the core correctness story: this PR fixes a genuine false-positive in orphan detection and is a net improvement. Four of five active panelists produced only nits. The single blocking finding comes from devx-ux-expert and it is legitimate: _expand_with_ancestors unconditionally adds every ancestor prefix to the allowed set, which means a real standalone package installed at owner/repo will be silently spared by prune and deps list if any declared subdirectory dependency under that repo is also present. This is not a theoretical concern -- it is the exact category of silent miss that prune is designed to prevent, and it inverts the safety guarantee from conservative (never miss an orphan) to optimistic (trust that ancestors are always intermediaries). Supply-chain-security-expert surfaced the same structural risk from a different angle (the unconditional whitelist) and flagged the missing path-traversal guard as a secondary hardening concern; both findings reinforce the devx-ux-expert required item. The fix path is clear and bounded: before adding an ancestor prefix to the expanded set, verify it is not itself present in the scanned installed packages unless it is also independently in the declared expected set.

The python-architect and cli-logging-expert nits are housekeeping that should accompany any rework: tighten the return type to set[str], correct the docstring example, align orphan-warning phrasing between prune.py and deps/cli.py, and migrate the direct console.print in deps/cli.py to CommandLogger. The ADO three-level path edge case is worth a comment and a targeted test but does not block the fix. The supply-chain nit about the implicit safety invariant should be documented inline so a future install-strategy refactor cannot silently break it.

The CHANGELOG entry should be rewritten before merge. The current text is accurate but written for maintainers. The user-facing framing should open with the symptom and close with the outcome. This matters: mono-repo skill authors are early adopters and evangelists, and a prune command that silently destroys their layout is high-severity friction for the segment most likely to drive word-of-mouth.

Dissent resolved: No panelist disagreed on required vs nit classification. Supply-chain-security-expert and devx-ux-expert both identified the unconditional ancestor whitelist as the central risk but framed it differently -- devx as a user-visible false negative, supply-chain as a structural invariant dependency. The two framings are complementary, not in conflict. The devx required finding subsumes the supply-chain nit; resolving it resolves both.

Growth/positioning note: Mono-repo skill authoring -- a single repo containing multiple skills installed via subdirectory paths -- is a power-user pattern that unlocks significant workflow density. Once the false-negative guard is in place, the next release post should include a one-line callout aimed at this segment: "Mono-repo skill authors: apm prune is now safe to run in CI." This is a concrete, verifiable claim that costs nothing to add and directly addresses the credibility gap that early adopters report when recommending APM to their teams.

---

Per-persona findings (full)

Python Architect

classDiagram
    direction LR
    class helpers_module {
        <<Module>>
        +_build_expected_install_paths(declared_deps, lockfile, dir) set
        +_scan_installed_packages(apm_modules_dir) list
        +_check_orphaned_packages() list
        +_expand_with_ancestors(paths) set
    }
    class prune_cmd {
        <<IOBoundary>>
        +prune(ctx, dry_run) None
    }
    class deps_cli_mod {
        <<IOBoundary>>
        +_resolve_scope_deps(apm_dir, logger) dict
    }
    class helpers_module:::touched
    classDef touched fill:#fff3b0,stroke:#d47600
    note for helpers_module "_expand_with_ancestors: Pure Function\nstateless, no I/O, 3 call sites (rule: abstract when 3+)"
    prune_cmd ..> helpers_module : imports
    deps_cli_mod ..> helpers_module : imports

Loading

flowchart TD
    A["apm prune / apm deps list / apm compile"] --> B["_build_expected_install_paths\n_helpers.py [I/O]"]
    B --> C["_scan_installed_packages\n_helpers.py [FS] rglob apm_modules/"]
    C --> D["_expand_with_ancestors(expected)\n_helpers.py [Pure] NEW"]
    D --> E["expected_with_ancestors: set[str]\n= leaf paths + all 2+-segment prefixes"]
    E --> F{"p in expected_with_ancestors?"}
    F -- "yes (leaf or ancestor)" --> G["skip: recognized install or intermediate dir"]
    F -- "no" --> H["append to orphaned_packages list"]
    H --> I["prune: safe_rmtree / deps: mark source=orphaned"]

Loading

Design patterns

• Used in this PR: Pure Function -- _expand_with_ancestors in _helpers.py is stateless and side-effect-free; extracted to _helpers.py because it is now called from 3 independent command paths, meeting the codebase rule of abstracting at 3+ call sites.
• Pragmatic suggestion: none -- the current shape is the simplest correct design at this scope.

Nits:

• Return type annotation on _expand_with_ancestors is unparameterized (-> set loses element type). Suggest: -> set[str]. (src/apm_cli/commands/_helpers.py:161)
• Docstring example only mentions owner/repo but the function actually returns all four intermediate prefixes. Suggest updating to enumerate all returned members. (src/apm_cli/commands/_helpers.py:163)
• ADO 3-level paths (org/project/repo) add org/project to the ancestor set; if a standalone orphan were installed there, it would be missed. Suggest a comment and targeted test. (src/apm_cli/commands/_helpers.py:171)

CLI Logging Expert

Nits:

• Orphan-warning phrasing is inconsistent: prune.py emits "Found N orphaned package(s):" while deps/cli.py emits "N orphaned package(s) found (not in apm.yml):". Suggest aligning to one canonical phrase. (src/apm_cli/commands/prune.py)
• deps/cli.py orphan output block uses console.print directly instead of delegating through CommandLogger -- pre-existing anti-pattern, natural cleanup opportunity. (src/apm_cli/commands/deps/cli.py)
• Test comment explains old behavior rationale; could also note that the string is the canonical orphan-header and the coupling is intentional. (tests/unit/test_deps_list_tree_info.py)

DevX UX Expert

Required:

• Ancestor expansion can silently suppress a real orphan -- see Required section above.

Nits:

• CHANGELOG leads with apm compile; reorder to apm prune, apm deps list, apm compile for user-familiarity. (CHANGELOG.md)
• No docs update needed: no CLI command surface or flag changed.

Supply Chain Security Expert

No required findings.

Nits:

• _expand_with_ancestors has no explicit traversal guard; a future caller passing paths not pre-filtered by relative_to() could expand ..-containing entries. Suggest adding if '..' in p.split('/'): continue guard. (src/apm_cli/commands/_helpers.py)
• Ancestor suppression is only safe because get_install_path() anchors installs at the 2-segment repo root; this invariant is not documented inline and a future install-strategy change could silently break it. (src/apm_cli/commands/_helpers.py)
• except Exception: return [] in _check_orphaned_packages now also swallows bugs in _expand_with_ancestors; consider narrowing the try/except scope so expansion errors propagate. (src/apm_cli/commands/_helpers.py)

Auth Expert

Inactive -- No auth-related files touched; the PR modifies only orphan-detection path expansion in _helpers.py, deps/cli.py, and prune.py with no changes to authentication, token management, or credential resolution.

OSS Growth Hacker

No required findings.

Nits:

• CHANGELOG entry is written for maintainers: leads with internal terms (dict-form, git: + path:, ancestor of the expected install path). Suggest rewriting to open with the user-visible symptom: "Fixed: apm prune, apm deps list, and apm compile no longer delete or flag as orphaned the parent directory of a skill installed from a subdirectory path (e.g. .apm/skills/skill-name). The folder is now correctly recognized as part of the install tree. ([BUG] apm compile falsely flags parent directory of subdirectory virtual package as orphaned #1050)" (CHANGELOG.md)
• This fix unblocks mono-repo skill authors from safely running apm prune in CI -- worth a one-line callout in the next release post aimed at that segment.

_{Verdict computed deterministically: 1 required finding across 5 active panelists. APPROVE iff N == 0. Push a new commit to clear this verdict label automatically.}

Note

🔒 Integrity filter blocked 2 items

The following items were blocked because they don't meet the GitHub integrity level.

• #1052 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• Don't flag parent dirs of subdirectory packages as orphaned #1052 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review Panel for issue #1052 · ● 1.8M · ◷

[github-actions]

APM Review Panel Verdict: REJECT

Three specialists converged on one clear correctness bug in prune.py; two security findings add required scope; CLI output issues in deps/cli.py introduce a latent crash path now left exposed; growth findings are reclassified as nits per persona contract.

Required before merge (9 items)

• [python-architect] prune.py omits standalone_installed guard, creating a real-orphan false-negative at src/apm_cli/commands/prune.py

  • Why: _check_orphaned_packages (advisory) filters installed to paths with apm.yml before calling _expand_with_ancestors(expected, standalone_installed). prune.py calls _expand_with_ancestors(expected_installed) with no second argument, so a genuinely orphaned owner/repo package (with its own apm.yml) is silently suppressed from deletion whenever any declared dep is installed under owner/repo/subpath. A destructive command (rm -rf) behaving differently from its advisory display path is a correctness bug.
  • Suggested fix: Compute standalone_installed = [p for p in installed_packages if (apm_modules_dir / p / APM_YML_FILENAME).exists()] and pass it: expected_with_ancestors = _expand_with_ancestors(expected_installed, standalone_installed).

• [cli-logging-expert] Orphan warning block in deps/cli.py bypasses CommandLogger (anti-pattern) at src/apm_cli/commands/deps/cli.py:288

  • Why: The orphan warning block uses console.print(..., style='yellow') and click.echo() directly rather than routing through CommandLogger.warning(). This PR touches this code path without addressing the architectural violation. prune.py correctly uses logger.warning/progress/success, creating an inconsistency for the same concept across two commands. Every command must route output through CommandLogger per APM Output Architecture.
  • Suggested fix: Replace console.print/click.echo orphan blocks with logger.warning(...) calls through a CommandLogger instance, matching prune.py's approach.

• [cli-logging-expert] [!] string literal passed to console.print() without markup=False at src/apm_cli/commands/deps/cli.py:289

  • Why: Rich interprets square-bracket sequences as markup tags. [!] is not a valid tag and may raise a MarkupError or silently mangle output depending on Rich version. STATUS_SYMBOLS constants exist precisely to avoid embedding raw symbol strings in Rich calls. This is a latent crash path now touched and left exposed.
  • Suggested fix: Either pass markup=False to console.print(), use console.print(Text(...)), or route through CommandLogger which handles this safely.

• [supply-chain-security-expert] prune.py calls _expand_with_ancestors without standalone_installed -- real-orphan safety not applied to actual deletions at src/apm_cli/commands/prune.py:64

  • Why: A malicious package at owner/repo that carries a .apm/ subtree (making it discoverable by _scan_installed_packages) but no apm.yml (keeping it out of standalone_installed) will be silently suppressed from deletion whenever any declared dep is installed under owner/repo/subpath. The fix exists in the helper but is not threaded through to the command that actually removes packages.
  • Suggested fix: Mirror _check_orphaned_packages: compute standalone_installed = [p for p in installed_packages if (apm_modules_dir / p / APM_YML_FILENAME).exists()] and pass it as the second argument.

• [supply-chain-security-expert] Traversal guard splits only on / -- backslash-containing package names bypass the .. check at src/apm_cli/commands/_helpers.py:210

  • Why: _expand_with_ancestors does p.split('/') then checks '..' in parts. A path segment like owner\\..\\.\\evil would appear as a single unsplit token, pass the guard, and be emitted as an ancestor. path_security.validate_path_segments normalises backslashes before splitting, but _expand_with_ancestors does not call it. Current callers produce POSIX strings, so the risk is latent today, but the function carries no documented precondition and a future caller passing raw user input would be silently unprotected.
  • Suggested fix: Add p = p.replace('\\\\', '/') before the split, or call validate_path_segments(p, context='install path') and skip the path on PathTraversalError.

• [supply-chain-security-expert] apm.yml-existence heuristic for "standalone" packages is forgeable in both directions at src/apm_cli/commands/_helpers.py:282

  • Why: Suppression via absence -- a malicious package at owner/repo with only a .apm/ marker (no apm.yml) is not in standalone_installed; if a declared dep resolves to owner/repo/subpath, ancestor expansion unconditionally adds owner/repo to the expanded set, hiding the malicious package from orphan detection entirely. A cross-check against the lockfile dep keys would be tamper-evident.
  • Suggested fix: Replace the apm.yml-existence heuristic with a lockfile membership check so the determination is tamper-evident and consistent with the lockfile-is-canonical invariant.

• [supply-chain-security-expert] Ancestor suppression creates a structural orphan-hiding primitive exploitable via subdirectory dep declaration at src/apm_cli/commands/_helpers.py:161

  • Why: Any path owner/repo/... declared as a dep causes owner/repo to be added to the expanded set. An attacker who can influence the victim's apm.yml can make owner/repo permanently invisible to orphan detection. Bounding ancestor depth to the known install-root depth (2 segments for GitHub, 3 for ADO) limits the suppression surface without losing correctness.
  • Suggested fix: Cap ancestor generation at the known install-root depth and document the depth invariant as a security contract in the function docstring.

• [oss-growth-hacker] CHANGELOG entry is maintainer-voice, not user-voice at CHANGELOG.md

  • Why: Entry opens with "Orphan detection improved" -- a technical label, not a user outcome. The target reader is someone who hit this bug and wondered why apm prune was yelling at them. The hook should lead with the pain removed, not the mechanism fixed.
  • Suggested fix: Reframe around the user moment: "apm prune no longer flags directories it put there itself -- skills installed from a subdirectory path (e.g., owner/repo/.apm/skills/skill-name) no longer cause the parent owner/repo/ clone to appear as an orphan. Fixes spurious removal prompts in multi-skill and monorepo-style setups. ([BUG] apm compile falsely flags parent directory of subdirectory virtual package as orphaned #1050)"

• [oss-growth-hacker] No social/release story surfaced for multi-skill monorepo support

  • Why: This fix is a meaningful maturity signal -- apm now correctly handles monorepo/multi-skill repo patterns without confusing its own artifacts for user errors. That story is missing from the PR description, CHANGELOG, and any release notes framing.
  • Suggested fix: Add a one-paragraph release note angle: "If you maintain a repo that serves multiple AI skills from subdirectories, apm prune now understands the full install tree and won't ask you to remove directories it needs."

Nits (14 items, skip if you want)

• [python-architect] deps/cli.py orphan display shares the same missing installed= guard as prune.py -- display-only inconsistency but worth fixing for correctness parity at src/apm_cli/commands/deps/cli.py
• [python-architect] No direct unit tests for _expand_with_ancestors with the installed= parameter; the guard logic is only tested indirectly at tests/unit/test_command_helpers.py
• [python-architect] ADO 3-segment invariant is load-bearing but only documented in a docstring; consider an inline comment near the range(2, len(parts)) loop at src/apm_cli/commands/_helpers.py
• [python-architect] builtins.set[str] return annotation is correct (module-level shadow) but non-obvious; a brief inline comment would help readers at src/apm_cli/commands/_helpers.py
• [cli-logging-expert] Inconsistent phrasing for orphan count across prune vs deps: prune.py says "Found {n} orphaned package(s):" while deps/cli.py says "{n} orphaned package(s) found (not in apm.yml):" -- standardise at src/apm_cli/commands/prune.py:72
• [cli-logging-expert] Test assertion "orphaned package(s) found" matches deps output but not prune output -- a one-line comment clarifying the assertion targets the deps-specific header would prevent confusion at tests/unit/test_deps_list_tree_info.py:174
• [devx-ux-expert] CHANGELOG phrasing "a parent directory you cannot remove" is colloquial but slightly misleading -- "intermediate directories of subdirectory-installed packages" is more precise at CHANGELOG.md
• [devx-ux-expert] CHANGELOG could note "No changes to apm.yml or the lockfile are required to benefit from this fix" to reduce friction for upgraders at CHANGELOG.md
• [devx-ux-expert] test_list_subdirectory_parent_not_orphaned places apm.yml in owner/repo, conflating the filesystem-intermediary scenario with the real-orphan scenario; the simpler fixture (no apm.yml in owner/repo) better represents the bug being fixed at tests/unit/test_deps_list_tree_info.py:296
• [supply-chain-security-expert] _check_orphaned_packages has doubly-silent except Exception: return [] blocks; a corrupted lockfile causes all orphan advisories to be suppressed without any user-visible signal at src/apm_cli/commands/_helpers.py:256
• [supply-chain-security-expert] Orphan list ordering is non-deterministic across runs due to rglob traversal order; sorting the result produces stable, diffable output at src/apm_cli/commands/_helpers.py:220
• [oss-growth-hacker] apm prune output could name the reason a kept path was retained (e.g. in --verbose mode: "Keeping owner/repo/ -- referenced by owner/repo/.apm/skills/my-skill") at src/apm_cli/commands/prune.py
• [oss-growth-hacker] CHANGELOG buries the multi-command scope; "The same fix applies to apm deps list and apm compile" should appear earlier at CHANGELOG.md
• [oss-growth-hacker] "Orphan detection" is jargon; "false removal prompts" or "phantom orphans" would travel better in release communications at CHANGELOG.md

CEO arbitration

Three panelists independently flagged the missing standalone_installed guard in prune.py as a correctness defect: python-architect (required), supply-chain-security-expert (required), and devx-ux-expert (nit). The tiebreak is unambiguous -- a destructive command (rm -rf) that silently skips a real orphan because its advisory display path does not is a correctness bug, not a style concern. The fix is mechanical: compute the filtered list identically to _check_orphaned_packages and pass it as the second argument. The same gap exists in deps/cli.py (display-only, non-destructive) and should be corrected for consistency, but it does not independently block merge.

The CLI-logging-expert correctly identifies that the orphan warning block in deps/cli.py bypasses CommandLogger and that the [!] literal risks a MarkupError. Both are real defects. Neither was introduced by this PR -- they are pre-existing in the touched code path. The MarkupError risk is required because it is a latent crash path now touched and left exposed; the broader CommandLogger architecture alignment is a tracked follow-up issue, not a merge blocker for this PR. The supply-chain backslash traversal bypass and apm.yml forgeability are real but latent: current callers produce POSIX strings, and influencing apm.yml is already a high-privilege operation. The ancestor suppression depth cap is sound defensive design. All three are required fixes, but they predate this PR and the lockfile-membership refactor is out of scope for a single bug fix PR; they should ship as a focused follow-up with an explicit security contract. They remain required in the tally because they are real, but the maintainer should weigh opening tracking issues as an acceptable resolution path.

The growth hacker's two required items -- CHANGELOG voice and monorepo release framing -- are legitimate positioning observations but fall outside the specialist blocking threshold per the OSS Growth Hacker persona contract ("you do not block specialist findings"). They are reclassified as nits for purposes of merge decisions. The CHANGELOG rewrite suggestion is concrete and low-cost and should be adopted before the release post ships.

Dissent resolved: DevX-UX-expert classified the prune.py standalone_installed gap as a nit; python-architect and supply-chain-security-expert classified it as required. CEO overrules DevX: destructive vs advisory behavioral divergence on the same correctness invariant is a required fix. OSS-growth-hacker's two required items are reclassified as nits per persona contract -- growth findings do not block specialist rulings absent a concrete case that outweighs the specialist position.

Growth/positioning note: This fix is quietly the most important positioning commit in recent history: apm now correctly handles monorepo and multi-skill repo layouts without false prune prompts. That is table-stakes functionality that competing package managers get wrong constantly. A short post demonstrating a two-skill monorepo workflow would convert this bug fix into a community acquisition event. Do not bury it in a patch entry.

---

Per-persona findings (full)

Python Architect

flowchart TD
    A["declared deps + lockfile"] --> B["_build_expected_install_paths()\n-> expected: set[str]"]
    B --> C["_expand_with_ancestors(expected, standalone_installed)\n-> expected_with_ancestors: set[str]"]
    D["apm_modules/ filesystem"] --> E["_scan_installed_packages()\n-> installed: list[str]"]
    E --> F["standalone_installed\n(filter: has apm.yml)"]
    F --> C
    E --> G["orphaned = installed - expected_with_ancestors"]
    C --> G
    G --> H{"caller"}
    H -->|"_check_orphaned_packages"| I["compile warning"]
    H -->|"prune (NEEDS FIX)"| J["rm -rf"]
    H -->|"deps/cli"| K["display tree"]

Loading

flowchart LR
    subgraph correct ["Correct (real-orphan safe)"]
        direction TB
        COH["_check_orphaned_packages"] -->|"passes standalone_installed"| EWA1["_expand_with_ancestors"]
    end
    subgraph missing ["Missing guard"]
        direction TB
        PR["prune.py"] -->|"NO installed= arg"| EWA2["_expand_with_ancestors"]
        DC["deps/cli.py"] -->|"NO installed= arg"| EWA3["_expand_with_ancestors"]
    end

Loading

Design patterns

• Used in this PR: Collect-then-render -- _expand_with_ancestors materialises ancestors once for O(1) membership checks at all call sites, consistent with DiagnosticCollector pattern
• Pragmatic suggestion: none -- the current shape is the simplest correct design at this scope

Required:

• prune.py omits standalone_installed guard, creating a real-orphan false-negative at src/apm_cli/commands/prune.py (see above)

Nits:

• deps/cli.py orphan display shares the same missing installed= guard (display-only)
• No direct unit tests for _expand_with_ancestors with installed= parameter
• ADO 3-segment invariant load-bearing but only in docstring
• builtins.set[str] annotation non-obvious without inline comment

CLI Logging Expert

Required:

• Orphan warning block in deps/cli.py bypasses CommandLogger at src/apm_cli/commands/deps/cli.py:288
• [!] symbol passed to console.print() without markup=False at src/apm_cli/commands/deps/cli.py:289

Nits:

• Inconsistent phrasing for orphan count across prune vs deps
• Test assertion string matches deps output but not prune output -- add clarifying comment

DevX UX Expert

No findings required.

Nits:

• CHANGELOG phrasing "a parent directory you cannot remove" is colloquial but imprecise
• No user-visible output change documented for upgraders with workarounds
• prune.py does not pass standalone_installed to _expand_with_ancestors (consistency with _check_orphaned_packages)
• test_list_subdirectory_parent_not_orphaned conflates intermediary vs standalone scenarios in its fixture

Supply Chain Security Expert

Required:

• prune.py calls _expand_with_ancestors without standalone_installed at src/apm_cli/commands/prune.py:64
• Traversal guard splits only on '/' -- backslash bypass at src/apm_cli/commands/_helpers.py:210
• apm.yml-existence heuristic is forgeable at src/apm_cli/commands/_helpers.py:282
• Ancestor suppression creates orphan-hiding primitive; cap at install-root depth at src/apm_cli/commands/_helpers.py:161

Nits:

• Doubly-silent exception swallowing in _check_orphaned_packages at line 256
• Orphan list ordering non-deterministic across runs (rglob traversal)

Auth Expert

Inactive -- No auth-related files changed; PR only modifies orphan-detection logic in _helpers.py, deps/cli.py, and prune.py, which do not touch authentication, token management, credential resolution, or host classification.

OSS Growth Hacker

Required (reclassified as nits by CEO per persona contract):

• CHANGELOG entry is maintainer-voice, not user-voice at CHANGELOG.md
• No social/release story surfaced for multi-skill monorepo support

Nits:

• apm prune output could name reason a path is kept (verbose-mode opportunity)
• CHANGELOG buries multi-command scope (deps list, compile)
• "Orphan detection" is jargon; "false removal prompts" travels better

_{Verdict computed deterministically: 9 required findings across 4 active panelists. APPROVE iff N == 0. Push a new commit to clear this verdict label automatically.}

Note

🔒 Integrity filter blocked 2 items

The following items were blocked because they don't meet the GitHub integrity level.

• #1052 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• Don't flag parent dirs of subdirectory packages as orphaned #1052 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review Panel for issue #1052 · ● 2M · ◷

[github-actions]

APM Review Panel Verdict: REJECT

The core orphan-detection fix is directionally correct and strategically important, but two security findings and a cross-command logging-severity gap block merge. Remediations are narrow and surgical -- no design reversal required.

Required before merge (4 items)

• [supply-chain-security-expert] Ad-hoc .. traversal check bypasses the canonical path_security.validate_path_segments guard at src/apm_cli/commands/_helpers.py

  • Why: The security model mandates all path validation flow through validate_path_segments. The hand-rolled ".." in parts check misses single-dot segments (owner/./repo), which validate_path_segments rejects by default. A dep declaration with a . segment slips past the guard and produces unexpected ancestor paths.
  • Suggested fix: Replace the hand-rolled guard with try: validate_path_segments(p, context='ancestor expansion') except PathTraversalError: continue. Import PathTraversalError and validate_path_segments from apm_cli.utils.path_security.

• [supply-chain-security-expert] Bare except Exception in _standalone_installed_packages silently destroys lockfile_keys, failing open on lockfile corruption at src/apm_cli/commands/_helpers.py

  • Why: The lockfile is the tamper-evident integrity primitive. Any exception -- including one triggered by an attacker-crafted lockfile -- resets lockfile_keys to empty and falls back to the weaker apm.yml-existence heuristic. A package in the lockfile but without an apm.yml (e.g., a git-dep) fails the standalone test and becomes eligible for ancestor suppression, hiding a real orphan.
  • Suggested fix: Replace except Exception: lockfile_keys = set() with except (AttributeError, TypeError, KeyError): lockfile_keys = set(). Let unexpected exceptions propagate to the outer _check_orphaned_packages handler.

• [cli-logging-expert] prune.py announces orphaned packages at progress/info level while deps/cli.py uses warning for the same condition at src/apm_cli/commands/prune.py:85

  • Why: Same event, different severity. prune.py uses logger.progress() (blue [i] prefix) while deps/cli.py correctly uses logger.warning() (yellow). A destructive command must be at least as loud as an advisory display command. A visually weaker signal on the destructive path is a user-trust defect.
  • Suggested fix: Change logger.progress(f'Found {len(orphaned_packages)} orphaned package(s):') to logger.warning(f'Found {len(orphaned_packages)} orphaned package(s):') in prune.py.

• [devx-ux-expert] logger.progress used for orphan list items and the apm prune recovery hint -- wrong semantic channel at src/apm_cli/commands/deps/cli.py

  • Why: logger.progress means "something is happening right now" (in-flight step). Orphan package names and the Run 'apm prune' recovery hint are static advisory context. In CI/quiet mode, progress lines may be suppressed, silently dropping the hint. Users may also see a misleading visual treatment.
  • Suggested fix: Replace logger.progress(f' - {pkg}') with logger.warning(f' - {pkg}') and logger.progress("Run 'apm prune'...") with logger.info("Run 'apm prune' to remove orphaned packages").

Nits (15 items, skip if you want)

• [python-architect] builtins.set / builtins.list in new helpers is redundant after module-level restoration at src/apm_cli/commands/_helpers.py
• [python-architect] Depth-cap magic number 4 in range(2, min(4, len(parts))) should be a named constant at src/apm_cli/commands/_helpers.py
• [python-architect] _standalone_installed_packages: lockfile param has no type annotation at src/apm_cli/commands/_helpers.py
• [python-architect] Nested try/except in deps/cli.py obscures which operation failed; add logger.debug(f'orphan-check setup failed: {exc}') at src/apm_cli/commands/deps/cli.py
• [python-architect] _expand_with_ancestors and _standalone_installed_packages are domain logic living in the commands layer; consider future move to src/apm_cli/core/orphan_detection.py at src/apm_cli/commands/_helpers.py
• [cli-logging-expert] logger.progress for Run 'apm prune' hint downgrades an actionable recovery step below the severity of its warning at src/apm_cli/commands/deps/cli.py
• [cli-logging-expert] Bullet change from * to - is undocumented; verify codebase-wide consistency at src/apm_cli/commands/deps/cli.py
• [devx-ux-expert] Leading newline before Run 'apm prune' hint silently dropped; add spacing so hint does not run against the last package bullet at src/apm_cli/commands/deps/cli.py
• [devx-ux-expert] Bullet changed from * to - with no mention in CHANGELOG; users who parse orphan output may be surprised at CHANGELOG.md
• [supply-chain-security-expert] Backslash-normalized ancestor strings added to expanded but original p (with backslashes) kept in materialized_set unchanged; normalize before insertion to prevent silent membership-check mismatches at src/apm_cli/commands/_helpers.py
• [supply-chain-security-expert] deps/cli.py outer except Exception: standalone_installed_for_check = [] fails open for the same reason as the _helpers.py catch; narrow to (AttributeError, TypeError, KeyError) at src/apm_cli/commands/deps/cli.py
• [supply-chain-security-expert] Depth cap 4 in min(4, len(parts)) is a load-bearing magic number; define ANCESTOR_DEPTH_CAP = 4 at module level at src/apm_cli/commands/_helpers.py
• [oss-growth-hacker] CHANGELOG entry buries the user benefit after implementation detail; lead with the user outcome sentence at CHANGELOG.md
• [oss-growth-hacker] No docs or quickstart callout for the monorepo/subdirectory skill distribution pattern this fix enables; consider a dedicated layout doc
• [oss-growth-hacker] Release note lacks a before/after UX illustration; a one-liner ("Before: apm prune prompted to remove owner/repo/ even when you installed it. After: no prompt.") makes it shareable at CHANGELOG.md

CEO arbitration

This PR solves a genuine and concrete bug: parent directories of legitimately installed subdirectory packages were being flagged as orphans, creating false positives in a destructive command (apm prune). The core fix is directionally correct and strategically important for monorepo adopters. However, the panel surfaced four required findings that collectively represent a meaningful risk surface before this ships. Two are security concerns that cannot be waived: the ad-hoc .. traversal check bypasses the canonical validate_path_segments guard, leaving single-dot segments undetected; and the bare except Exception catch in _standalone_installed_packages silently swallows all lockfile errors, causing the function to fail open and making eligible orphans invisible on a corrupted or attacker-crafted lockfile. Both ask for narrow, surgical remediations -- not architectural rewrites -- so they are not reasons to reject the concept, only the current implementation. The two remaining required findings concern logging severity and are raised in alignment by cli-logging-expert and devx-ux-expert: the orphan-found signal in prune.py is emitted at progress/info level while the same event in deps/cli.py uses warning, and individual orphan bullet lines in deps/cli.py are routed through logger.progress (an in-flight-step channel) rather than a static advisory channel.

The python-architect nits are internally consistent and well-reasoned but none individually rise to required status. The depth-cap note from supply-chain-security-expert overlaps with the architect's and the correct resolution is to name the constant AND tie its value to the ADO root-depth invariant. The UX nits around bullet character change and CHANGELOG entry quality are valid and easy to fold into the fix pass.

Strategic read: this is a solid fix whose implementation needs one focused pass to close the security and logging gaps. The concept is low-risk and high-value for the target segment. The panel's required findings are actionable and bounded; no specialist is asking for a design reversal.

Dissent resolved: cli-logging-expert and devx-ux-expert agree logger.progress is wrong in both files but split on which file carries a REQUIRED finding. cli-logging-expert marks prune.py as required (destructive command must be loud) and treats deps/cli.py progress calls as a nit. devx-ux-expert marks deps/cli.py as required (static advisory context must not use an in-flight-step channel). I side with both simultaneously: both are required. Orphan-found announcements and per-item orphan list use logger.warning; the recovery hint uses logger.info.

Growth/positioning note: This fix quietly enables a first-class layout (owner/repo/.apm/skills/skill-name) increasingly common in the GitHub AI ecosystem. Once the required findings are addressed and the fix ships cleanly, this warrants a targeted CHANGELOG headline and a docs entry for the subdirectory skill distribution pattern as a supported, named layout. The before/after UX illustration suggested by oss-growth-hacker would materially improve the CHANGELOG entry and should be incorporated in the same release rather than as a follow-up.

---

Per-persona findings (full)

Python Architect

classDiagram
    direction LR

    class _helpers {
        <<Module / IOBoundary>>
        +_build_expected_install_paths(declared, lockfile, dir) set
        +_scan_installed_packages(dir) list
        +_check_orphaned_packages() list
    }
    class _expand_with_ancestors {
        <<Pure>>
        +__call__(paths, installed) set
    }
    class _standalone_installed_packages {
        <<Pure>>
        +__call__(installed, dir, lockfile) list
    }
    class _resolve_scope_deps {
        <<IOBoundary>>
        +__call__(apm_dir, logger, insecure_only) tuple
    }
    class prune_command {
        <<IOBoundary>>
        +run(apm_dir, logger) None
    }
    class LockFile {
        <<ValueObject>>
        +dependencies dict
        +read(path) LockFile
    }
    class CommandLogger {
        <<Base>>
        +warning(msg) None
        +progress(msg) None
    }

    _helpers *-- _expand_with_ancestors : contains
    _helpers *-- _standalone_installed_packages : contains
    _helpers ..> LockFile : reads
    _resolve_scope_deps ..> _expand_with_ancestors : calls
    _resolve_scope_deps ..> _standalone_installed_packages : calls
    _resolve_scope_deps ..> CommandLogger : emits via
    prune_command ..> _expand_with_ancestors : calls
    prune_command ..> _standalone_installed_packages : calls
    prune_command ..> CommandLogger : emits via

Loading

flowchart TD
    A([apm deps list / apm prune]) --> B[_resolve_scope_deps / prune.run]
    B --> C["[FS] apm_modules_path.rglob - scan candidates"]
    C --> D{candidate valid?}
    D -- skip --> C
    D -- keep --> E["scanned_candidates.append"]
    E --> C
    C --> F["[I/O] get_lockfile_path + LockFile.read"]
    F --> G{lockfile exists?}
    G -- no --> H[lockfile_for_check = None]
    G -- yes --> I[lockfile_for_check = LockFile]
    H --> J[_standalone_installed_packages]
    I --> J
    J --> K{lockfile key present?}
    K -- yes --> L[mark as standalone]
    K -- no --> M{"[FS] apm.yml exists?"}
    M -- yes --> L
    M -- no --> N[skip - filesystem intermediary]
    L --> O[standalone_installed_for_check]
    N --> O
    O --> P[_expand_with_ancestors]
    P --> Q{path contains .. after backslash-norm?}
    Q -- yes --> R[keep original, skip expansion]
    Q -- no --> S["range(2, min(4, len(parts))): emit ancestors up to depth 3"]
    S --> T{ancestor in installed_set AND not in declared?}
    T -- yes --> U[skip - would mask real orphan]
    T -- no --> V[add ancestor to expanded set]
    V --> W[declared_with_ancestors]
    R --> W
    U --> W
    W --> X["for each scanned candidate: org_repo_name not in declared_with_ancestors?"]
    X -- orphaned --> Y[orphaned_packages.append]
    X -- expected --> Z[installed_packages.append]
    Y --> AA["[OUT] logger.warning / logger.progress"]
    Z --> AA

Loading

No required findings.

Nits:

• builtins.set / builtins.list in new helpers is redundant after module-level restoration at src/apm_cli/commands/_helpers.py. Use plain set() and list() consistent with every other helper in the module.
• Depth-cap magic number 4 in range(2, min(4, len(parts))) should be a named constant. Add _ANCESTOR_DEPTH_CAP = 3 and write range(2, min(_ANCESTOR_DEPTH_CAP + 1, len(parts))).
• _standalone_installed_packages: lockfile param has no type annotation. Add lockfile: LockFile | None = None.
• Nested try/except in deps/cli.py obscures which operation failed. Add logger.debug(f'orphan-check setup failed: {exc}') in the except clause.
• _expand_with_ancestors and _standalone_installed_packages are domain logic living in the commands layer. Consider future extraction to src/apm_cli/core/orphan_detection.py.

CLI Logging Expert

Required:

• prune.py announces orphaned packages at progress/info level while deps/cli.py uses warning for the same condition at src/apm_cli/commands/prune.py:85. Change logger.progress(...) to logger.warning(...) for the orphan-found header.

Nits:

• logger.progress for Run 'apm prune' hint downgrades an actionable recovery step below the severity of its warning. Use logger.warning(...) so the fix stays in the same yellow channel.
• Bullet change from * to - is undocumented. Verify codebase-wide consistency.

DevX UX Expert

Required:

• logger.progress used for orphan list items and the apm prune recovery hint -- wrong semantic channel in src/apm_cli/commands/deps/cli.py. Replace logger.progress(f' - {pkg}') with logger.warning(f' - {pkg}') and the recovery hint with logger.info(...).

Nits:

• Leading newline before Run 'apm prune' hint silently dropped; add spacing.
• Bullet changed from * to - with no mention in CHANGELOG; add a one-liner in Changed.

Supply Chain Security Expert

Required:

• Ad-hoc .. traversal check bypasses validate_path_segments at src/apm_cli/commands/_helpers.py. Replace with try: validate_path_segments(p, context='ancestor expansion') except PathTraversalError: continue.
• Bare except Exception in _standalone_installed_packages silently destroys lockfile_keys, failing open. Narrow to except (AttributeError, TypeError, KeyError).

Nits:

• Backslash-normalized ancestor strings added to expanded but original p kept in materialized_set unchanged; normalize before insertion.
• deps/cli.py outer except Exception: standalone_installed_for_check = [] also fails open; narrow to (AttributeError, TypeError, KeyError).
• Depth cap 4 is a load-bearing magic number; define ANCESTOR_DEPTH_CAP = 4 at module level.

Auth Expert

Inactive -- No auth-sensitive files changed; PR modifies orphan detection path logic in _helpers.py, deps/cli.py, and prune.py with no changes to authentication, token management, credential resolution, or AuthResolver host classification.

OSS Growth Hacker

No required findings.

Nits:

• CHANGELOG entry buries the user benefit after implementation detail; lead with the user outcome sentence.
• No docs or quickstart callout for the monorepo/subdirectory skill distribution pattern this fix enables.
• Release note lacks a before/after UX illustration; a one-liner ("Before: apm prune prompted to remove owner/repo/ even when you installed it. After: no prompt.") makes it shareable.

Side-channel note: This fix is a quiet trust-builder for a high-value segment -- teams distributing skills from monorepos (common in the GitHub AI ecosystem). False positives in safety commands like apm prune are adoption killers. The growth opportunity is to frame this as a launch beat: "APM now fully supports monorepo-style skill distribution" with a concrete example layout in the docs.

_{Verdict computed deterministically: 4 required findings across 5 active panelists. APPROVE iff N == 0. Push a new commit to clear this verdict label automatically.}

Note

🔒 Integrity filter blocked 2 items

The following items were blocked because they don't meet the GitHub integrity level.

• Don't flag parent dirs of subdirectory packages as orphaned #1052 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• #1052 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review Panel for issue #1052 · ● 2.2M · ◷

[github-actions]

🔬 APM Review Panel — PR #1052

Verdict: ✅ APPROVED
Panel composition: Security · Correctness · Testing · Maintainability

---

Summary

This PR fixes a correctness bug where apm prune, apm deps list, and apm compile falsely flagged intermediate filesystem directories as orphaned packages when a subdirectory dependency (e.g. owner/repo/.apm/skills/name) was installed by cloning the full repo. The fix is well-structured, security-aware, and thoroughly tested.

---

🛡️ Security Panelist

Score: Approved with commendation

The PR addresses a non-trivial supply-chain surface and does so correctly:

• Depth cap (min(4, len(parts))) bounds ancestor suppression to depth 3 (GitHub 2-segment, ADO 3-segment roots). Attacker-controlled apm.yml entries cannot suppress detection of arbitrarily deep orphan paths. ✅
• Canonical traversal guard: routes through validate_path_segments() rather than a hand-rolled ".." in parts check. Single-dot segments (owner/./repo) are also rejected. ✅
• Backslash normalization before splitting closes the owner\\..\\evil/sub vector (prior ad-hoc split on / only would have treated the whole token as one segment). ✅
• Lockfile-first, apm.yml-fallback in _standalone_installed_packages: the tamper-evident record wins; forgeable apm.yml is only a fallback for legacy installs. ✅
• Narrow exception handling: only (AttributeError, TypeError, KeyError) are absorbed; RuntimeError and other unexpected corruption propagates — prevents silent fail-open on corrupted lockfiles. ✅
• Real-orphan protection: installed_set guard in _expand_with_ancestors prevents a genuine owner/repo standalone package from being masked by a sibling subdirectory dep declaring the same root. ✅

---

✅ Correctness Panelist

Score: Approved

The logic is correct across all scenarios:

Scenario	Before	After
Subdir dep owner/repo/.apm/skills/x	owner/repo falsely orphaned	Correctly recognized as filesystem intermediary
Real orphan alongside subdir dep	Might be missed	Correctly flagged (real-orphan guard)
Whole-repo dep owner/repo	Correct	Unchanged
ADO 3-segment org/project/repo	N/A	Correctly expands org/project as ancestor
Traversal path owner/../etc/passwd	N/A	Kept for membership, no ancestors generated

The sorted() calls on orphan lists produce deterministic output, which is good for UX and test stability.

The _check_orphaned_packages → prune.py → deps/cli.py consistency (all three paths use identical ancestor-expansion logic) eliminates a class of advisory/destructive command divergence bugs.

---

🧪 Testing Panelist

Score: Approved with commendation

Test coverage is exemplary for a security-sensitive change:

• TestExpandWithAncestors: covers empty input, 2-segment paths, ADO 3-segment paths, prefix non-collision, traversal (backslash + ..), depth cap, and the real-orphan installed guard.
• TestCheckOrphanedPackagesSubdirectoryAncestor: end-to-end integration with filesystem fixtures covering all four key scenarios including the critical regression case (real owner/repo package WITH apm.yml that shares a root with a subdir dep is correctly flagged).
• TestExpandWithAncestorsRoutesThroughCanonicalGuard: mock-based verification that validate_path_segments is actually called — proves the ad-hoc guard is gone.
• TestStandaloneInstalledDoesNotSwallowCorruption: tests both the corruption-propagation path (RuntimeError) and the intentional-absorption path (TypeError from dependencies = 42).
• test_orphan_announce_parity.py: new file pins the warning vs info logger channel mapping for both prune and deps list so future refactors cannot silently regress.
• test_deps_list_tree_info.py: regression test for the deps list path, with correct fixture (no apm.yml at owner/repo — pure filesystem intermediary).

One minor nit: test_real_orphan_at_owner_repo_with_sibling_subdir_dep in test_command_helpers.py is the most important regression test and the docstring explains it clearly. No change needed.

---

🔧 Maintainability Panelist

Score: Approved

• The security contract documented in _expand_with_ancestors's docstring is clear, complete, and will help future contributors understand why the depth cap exists before touching it.
• _standalone_installed_packages documents its failure mode and the rationale for the dual-signal approach.
• CommandLogger.info vs CommandLogger.progress semantic split is a forward-looking hook for quiet-mode policy — the current implementation is correctly conservative (both delegate to _rich_info).
• from collections.abc import Iterable is the correct Python 3.9+ import.
• Orphan list sorted for determinism — good housekeeping.

---

Panel Conclusion

All four panelists approve. The implementation is correct, security-hardened, and well-tested. The PR closes a real usability bug (spurious prune prompts in multi-skill/monorepo setups) while tightening the orphan-detection safety net for real orphans. No blocking issues found.

→ Recommend merge.

Note

🔒 Integrity filter blocked 2 items

The following items were blocked because they don't meet the GitHub integrity level.

• Don't flag parent dirs of subdirectory packages as orphaned #1052 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• #1052 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review Panel for issue #1052 · ● 430.5K · ◷