fix: virtual subdirectory deps marked as orphaned, skipping instruction processing

[sergio-sisternes-epam]

Problem

Virtual subdirectory packages (owner/repo/subdir) are incorrectly handled in two places, causing:

• apm deps list: dependency shown as orphaned
• apm deps tree: dependency shown as unknown
• apm compile --verbose: Instruction Processing: 0.0ms — no primitives discovered from the dependency

Root Cause

Both get_dependency_declaration_order() in discovery.py and orphan detection in deps.py flatten all virtual dependencies using get_virtual_package_name(), producing owner/virtual-pkg-name. However, DependencyReference.get_install_path() correctly differentiates virtual subdirectory packages (owner/repo/subdir) from virtual file/collection packages (owner/virtual-pkg-name).

This path-shape mismatch means the dependency name returned by discovery doesn't match the actual installed directory, so:

1. scan_dependency_primitives() can't resolve the dependency path → no primitives found
2. Orphan detection can't match declared deps to installed dirs → marks them as orphaned

Fix

• discovery.py: get_dependency_declaration_order() now checks is_virtual_subdirectory() and returns the natural path (owner/repo/subdir) instead of the flattened name.
• deps.py: Orphan detection uses the same subdirectory-aware logic when building the declared_deps set.
• Tests: Added regression tests for virtual subdirectory packages in test_virtual_package_orphan_detection.py.

Validation

• All 9 integration tests pass
• End-to-end: apm deps list no longer shows orphaned; apm compile --verbose shows Instruction Processing: 5.4ms (was 0.0ms)

Refs: #99

[Copilot]

Pull request overview

This PR fixes a critical bug where virtual subdirectory packages (e.g., owner/repo/skills/azure-naming) were incorrectly flattened using get_virtual_package_name(), causing them to be marked as orphaned in dependency views and skipping instruction processing during compilation.

Changes:

• Fixed path resolution in get_dependency_declaration_order() to preserve natural path structure for virtual subdirectory packages instead of flattening them
• Fixed orphan detection logic in deps list command to correctly recognize virtual subdirectory packages
• Added regression tests for virtual subdirectory package handling

Reviewed changes

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

File	Description
src/apm_cli/primitives/discovery.py	Updated get_dependency_declaration_order() to check is_virtual_subdirectory() and return natural paths (owner/repo/subdir) instead of flattened names
src/apm_cli/commands/deps.py	Updated orphan detection to use subdirectory-aware logic when building declared_deps set
tests/integration/test_virtual_package_orphan_detection.py	Added _find_installed_subdirectory_packages() helper and two regression tests for virtual subdirectory orphan detection and declaration order

[danielmeppiel]

Good root cause analysis and clean fix for discovery.py and deps.py — the is_virtual_subdirectory() branching is the right approach, and the joinpath(*parts) fix in the follow-up commit is solid.

Two remaining gaps before this can be merged:

1. cli.py prune command (line ~764) has the identical unfixed pattern — get_virtual_package_name() for all virtual packages without an is_virtual_subdirectory() check. This means apm prune would incorrectly remove subdirectory packages.

2. deps.py:list_packages() scanner only walks 2–3 directory levels under apm_modules/, so it won't discover subdirectory packages at 4+ levels (e.g., owner/repo/skills/azure-naming). The test validates via helpers that do handle deep traversal, but the production scanner doesn't.

Both are detailed in inline comments.

[danielmeppiel]

Thanks for addressing the previous feedback — both the cli.py prune fix and the rglob scanner refactor look right.

However, the rglob refactor in deps.py introduced an ADO source label regression. The old code had separate branches for GitHub (2-level) and ADO (3-level) packages, setting 'source': 'github' or 'source': 'azure-devops' respectively. The unified rglob scanner lost this distinction and hardcodes 'github' for all non-orphaned packages.

Failing tests (verified locally):

• test_ado_e2e::TestADODepsAndPrune::test_deps_list_shows_correct_path — expects Source: azure-devops
• test_ado_e2e::TestMixedDependencies::test_mixed_deps_list — expects ADO packages labeled azure-devops

Note: These tests weren't caught in CI due to a test harness bug on our side (test binary resolution bypassed the PR artifact). We've fixed that in PR #98 — not your fault, but the regression still needs fixing.

Suggested fix: In the scanner loop, detect ADO packages and set the source accordingly - see my following comment

[danielmeppiel]

Suggested fix for ADO source regression

File: src/apm_cli/commands/deps.py

The problem

Line 122 hardcodes 'github' as the source for every non-orphaned package:

# Line 122
'source': 'orphaned' if is_orphaned else 'github',

The old code had separate scanner branches for GitHub (2-level) and ADO (3-level), each setting the correct source. The rglob refactor unified them but lost the distinction.

Important: a path-depth heuristic (len(rel_parts) >= 3 → ADO) would be wrong here — GitHub subdirectory packages like owner/repo/skills/azure-naming also have 3+ levels.

The fix (2 changes, same file)

Change 1 — Line 58: Replace declared_deps = set() with a dict that tracks source per path:

# Line 58 — before:
declared_deps = set()

# Line 58 — after:
declared_sources = {}  # dep_path → 'github' | 'azure-devops'

Then every .add(path) in the loop below (lines 62–93) becomes a dict assignment. The source is derived from dep.is_azure_devops() which is already in scope:

# Example — lines 72-75, before:
declared_deps.add(
    f"{repo_parts[0]}/{repo_parts[1]}/{repo_parts[2]}/{dep.virtual_path}"
)

# After:
source = 'azure-devops' if dep.is_azure_devops() else 'github'
declared_sources[
    f"{repo_parts[0]}/{repo_parts[1]}/{repo_parts[2]}/{dep.virtual_path}"
] = source

Same pattern for every other .add(...) call in that block.

Change 2 — Lines 116 and 122: Look up source from dict:

# Line 116 — before:
is_orphaned = org_repo_name not in declared_deps
# Line 122 — before:
'source': 'orphaned' if is_orphaned else 'github',

# Line 116 — after:
is_orphaned = org_repo_name not in declared_sources
# Line 122 — after:
'source': 'orphaned' if is_orphaned else declared_sources.get(org_repo_name, 'github'),

That's it — no new imports, no new functions, no heuristics. dep.is_azure_devops() is the authoritative source of truth.

[sergio-sisternes-epam]

Fixed in bede7f9. Replaced declared_deps set with a declared_sources dict that maps each dep path to its source label, derived from dep.is_azure_devops(). The scanner now looks up the source via declared_sources.get(org_repo_name, 'github') instead of hardcoding it.

We didn't catch this locally because the ADO e2e tests (test_deps_list_shows_correct_path, test_mixed_deps_list) are skipped without a real ADO environment, and as you noted, CI wasn't testing the PR artifact due to the harness bug fixed in #98.