fix: preserve DependencyReference through download pipeline (#382) (#383)

* fix: preserve DependencyReference through download pipeline (#382)

Subdirectory packages on non-GitHub/ADO hosts (e.g. GitLab, Gitea) failed
because the download pipeline converted DependencyReference to a string
via str() and re-parsed it in download_package(). The str() → parse()
round-trip lost the repo_url / virtual_path boundary on generic hosts
where _detect_virtual_package() uses a dynamic min_base_segments heuristic.

Fix: pass the structured DependencyReference object through the pipeline
instead of flattening to string. Specifically:

- download_package() and resolve_git_reference() now accept
  Union[str, DependencyReference], skipping the parse when already
  structured
- build_download_ref() returns a DependencyReference (via
  dataclasses.replace) instead of a flat string
- All call sites in install.py and _utils.py updated to pass the
  object directly

Closes #382

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

* fix: address Copilot review feedback

- Normalize original_ref to string in resolve_git_reference() to avoid
  leaking DependencyReference objects into ResolvedReference.original_ref
- Replace shallow test with one that verifies DependencyReference.parse()
  is NOT called when passing a structured object to download_package()
- Update test_resolve_git_reference_branch to match normalized original_ref

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

---------

Co-authored-by: Sergio Sisternes <sergio.sisternes@epam.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: 3 people

src/apm_cli/commands/deps/_utils.py

@@ -242,7 +242,7 @@ def _update_single_package(package_name: str, project_deps: List, apm_modules_pa
         _rich_info(f"Updating {target_dep.repo_url}...")
 
         # Download latest version
-        package_info = downloader.download_package(str(target_dep), package_dir)
+        package_info = downloader.download_package(target_dep, package_dir)
 
         _rich_success(f"[+] Updated {target_dep.repo_url}")
 
@@ -285,7 +285,7 @@ def _update_all_packages(project_deps: List, apm_modules_path: Path):
 
         try:
             _rich_info(f"  Updating {dep.repo_url}...")
-            package_info = downloader.download_package(str(dep), package_dir)
+            package_info = downloader.download_package(dep, package_dir)
             updated_count += 1
             _rich_success(f"  [+] {dep.repo_url}")
 

src/apm_cli/commands/install.py

@@ -928,31 +928,23 @@ def download_callback(dep_ref, modules_dir):
                     return result_path
                 return None
 
-            # Build repo_ref string - include host for GHE/ADO/Artifactory, plus reference if specified
-            repo_ref = dep_ref.repo_url
-            if dep_ref.host and dep_ref.host not in ("github.com", None):
-                if dep_ref.artifactory_prefix:
-                    repo_ref = f"{dep_ref.host}/{dep_ref.artifactory_prefix}/{dep_ref.repo_url}"
-                else:
-                    repo_ref = f"{dep_ref.host}/{dep_ref.repo_url}"
-            if dep_ref.virtual_path:
-                repo_ref = f"{repo_ref}/{dep_ref.virtual_path}"
-
             # T5: Use locked commit if available (reproducible installs)
             locked_ref = None
             if existing_lockfile:
                 locked_dep = existing_lockfile.get_dependency(dep_ref.get_unique_key())
                 if locked_dep and locked_dep.resolved_commit and locked_dep.resolved_commit != "cached":
                     locked_ref = locked_dep.resolved_commit
 
-            # Priority: locked commit > explicit reference > default branch
+            # Build a DependencyReference with the right ref to avoid lossy
+            # str() → parse() round-trips (#382).
+            from dataclasses import replace as _dc_replace
             if locked_ref:
-                repo_ref = f"{repo_ref}#{locked_ref}"
-            elif dep_ref.reference:
-                repo_ref = f"{repo_ref}#{dep_ref.reference}"
+                download_dep = _dc_replace(dep_ref, reference=locked_ref)
+            else:
+                download_dep = dep_ref
 
             # Silent download - no progress display for transitive deps
-            result = downloader.download_package(repo_ref, install_path)
+            result = downloader.download_package(download_dep, install_path)
             # Capture resolved commit SHA for lockfile
             resolved_sha = None
             if result and hasattr(result, 'resolved_reference') and result.resolved_reference:
@@ -1367,9 +1359,7 @@ def _collect_descendants(node, visited=None):
                 resolved_ref = None
                 if dep_ref.reference and dep_ref.get_unique_key() not in _pre_downloaded_keys:
                     try:
-                        resolved_ref = downloader.resolve_git_reference(
-                            f"{dep_ref.repo_url}@{dep_ref.reference}"
-                        )
+                        resolved_ref = downloader.resolve_git_reference(dep_ref)
                     except Exception:
                         pass  # If resolution fails, skip cache (fetch latest)
 

src/apm_cli/deps/github_downloader.py

@@ -11,6 +11,7 @@
 from typing import Optional, Dict, Any, Callable
 import random
 import re
+from typing import Union
 import requests
 
 import git
@@ -648,11 +649,13 @@ def _clone_with_fallback(self, repo_url_base: str, target_path: Path, progress_r
 
         raise RuntimeError(error_msg)
 
-    def resolve_git_reference(self, repo_ref: str) -> ResolvedReference:
+    def resolve_git_reference(self, repo_ref: Union[str, "DependencyReference"]) -> ResolvedReference:
         """Resolve a Git reference (branch/tag/commit) to a specific commit SHA.
         
         Args:
-            repo_ref: Repository reference string (e.g., "user/repo#branch")
+            repo_ref: Repository reference — either a DependencyReference object
+                or a string (e.g., "user/repo#branch"). Passing the object
+                directly avoids a lossy parse round-trip for generic git hosts.
             
         Returns:
             ResolvedReference: Resolved reference with commit SHA
@@ -661,23 +664,29 @@ def resolve_git_reference(self, repo_ref: str) -> ResolvedReference:
             ValueError: If the reference format is invalid
             RuntimeError: If Git operations fail
         """
-        # Parse the repository reference
-        try:
-            dep_ref = DependencyReference.parse(repo_ref)
-        except ValueError as e:
-            raise ValueError(f"Invalid repository reference '{repo_ref}': {e}")
+        # Accept both string and DependencyReference to avoid lossy round-trips
+        if isinstance(repo_ref, DependencyReference):
+            dep_ref = repo_ref
+        else:
+            try:
+                dep_ref = DependencyReference.parse(repo_ref)
+            except ValueError as e:
+                raise ValueError(f"Invalid repository reference '{repo_ref}': {e}")
 
         # Default to main branch if no reference specified
         ref = dep_ref.reference or "main"
 
+        # Normalize to string for ResolvedReference.original_ref
+        original_ref_str = str(dep_ref)
+
         # Artifactory: no git repo to query, return ref-based resolution
         if dep_ref.is_artifactory() or (
             self._parse_artifactory_base_url()
             and self._should_use_artifactory_proxy(dep_ref)
         ):
             is_commit = re.match(r'^[a-f0-9]{7,40}$', ref.lower()) is not None
             return ResolvedReference(
-                original_ref=repo_ref,
+                original_ref=original_ref_str,
                 ref_type=GitReferenceType.COMMIT if is_commit else GitReferenceType.BRANCH,
                 resolved_commit=None,
                 ref_name=ref
@@ -765,7 +774,7 @@ def resolve_git_reference(self, repo_ref: str) -> ResolvedReference:
                 shutil.rmtree(temp_dir, ignore_errors=True)
 
         return ResolvedReference(
-            original_ref=repo_ref,
+            original_ref=original_ref_str,
             ref_type=ref_type,
             resolved_commit=resolved_commit,
             ref_name=ref_name
@@ -1767,7 +1776,7 @@ def _download_package_from_artifactory(
 
     def download_package(
         self, 
-        repo_ref: str, 
+        repo_ref: Union[str, "DependencyReference"], 
         target_path: Path,
         progress_task_id=None,
         progress_obj=None
@@ -1778,7 +1787,9 @@ def download_package(
         package structure instead of cloning the full repository.
         
         Args:
-            repo_ref: Repository reference string (e.g., "user/repo#branch" or "user/repo/path/file.prompt.md")
+            repo_ref: Repository reference — either a DependencyReference object
+                or a string (e.g., "user/repo#branch"). Passing the object
+                directly avoids a lossy parse round-trip for generic git hosts.
             target_path: Local path where package should be downloaded
             progress_task_id: Rich Progress task ID for progress updates
             progress_obj: Rich Progress object for progress updates
@@ -1790,11 +1801,14 @@ def download_package(
             ValueError: If the repository reference is invalid
             RuntimeError: If download or validation fails
         """
-        # Parse the repository reference
-        try:
-            dep_ref = DependencyReference.parse(repo_ref)
-        except ValueError as e:
-            raise ValueError(f"Invalid repository reference '{repo_ref}': {e}")
+        # Accept both string and DependencyReference to avoid lossy round-trips
+        if isinstance(repo_ref, DependencyReference):
+            dep_ref = repo_ref
+        else:
+            try:
+                dep_ref = DependencyReference.parse(repo_ref)
+            except ValueError as e:
+                raise ValueError(f"Invalid repository reference '{repo_ref}': {e}")
 
         # Handle virtual packages differently
         if dep_ref.is_virtual:
@@ -1829,12 +1843,12 @@ def download_package(
         # When ARTIFACTORY_ONLY is set but no Artifactory proxy matched, block direct git
         if self._is_artifactory_only():
             raise RuntimeError(
-                f"ARTIFACTORY_ONLY is set but no Artifactory proxy is configured for '{repo_ref}'. "
+                f"ARTIFACTORY_ONLY is set but no Artifactory proxy is configured for '{dep_ref}'. "
                 "Set ARTIFACTORY_BASE_URL or use explicit Artifactory FQDN syntax."
             )
 
         # Regular package download (existing logic)
-        resolved_ref = self.resolve_git_reference(repo_ref)
+        resolved_ref = self.resolve_git_reference(dep_ref)
 
         # Create target directory if it doesn't exist
         target_path.mkdir(parents=True, exist_ok=True)

src/apm_cli/drift.py

@@ -44,6 +44,7 @@
 from __future__ import annotations
 
 import builtins
+from dataclasses import replace as _dataclass_replace
 from pathlib import Path
 from typing import TYPE_CHECKING, Dict, List, Optional, Set
 
@@ -171,8 +172,13 @@ def build_download_ref(
     *,
     update_refs: bool,
     ref_changed: bool,
-) -> str:
-    """Build the download-ref string passed to the package downloader.
+) -> "DependencyReference":
+    """Build the dependency reference passed to the package downloader.
+
+    Returns a :class:`DependencyReference` (not a flat string) so that
+    structured fields like ``virtual_path`` survive the trip to
+    ``download_package()`` without a lossy ``str()`` → ``parse()``
+    round-trip.  See :issue:`382`.
 
     Uses the locked commit SHA for reproducibility, unless:
     * ``update_refs=True`` — intentional update run; use the manifest ref.
@@ -186,20 +192,11 @@ def build_download_ref(
                      this dependency.
 
     Returns:
-        A ref string suitable for ``GitHubPackageDownloader.download_package``.
+        A :class:`DependencyReference` suitable for
+        ``GitHubPackageDownloader.download_package``.
     """
-    download_ref = str(dep_ref)
     if existing_lockfile and not update_refs and not ref_changed:
         locked_dep = existing_lockfile.get_dependency(dep_ref.get_unique_key())
         if locked_dep and locked_dep.resolved_commit and locked_dep.resolved_commit != "cached":
-            # Include the host so the downloader can resolve the correct
-            # server (e.g. GitHub Enterprise custom domains).  Without it
-            # ``DependencyReference.parse()`` would fall back to github.com.
-            if dep_ref.host:
-                base_ref = f"{dep_ref.host}/{dep_ref.repo_url}"
-            else:
-                base_ref = dep_ref.repo_url
-            if dep_ref.virtual_path:
-                base_ref = f"{base_ref}/{dep_ref.virtual_path}"
-            download_ref = f"{base_ref}#{locked_dep.resolved_commit}"
-    return download_ref
+            return _dataclass_replace(dep_ref, reference=locked_dep.resolved_commit)
+    return dep_ref

tests/test_apm_package_models.py

@@ -1354,4 +1354,96 @@ def test_package_dataclass_with_type(self):
     def test_package_dataclass_type_defaults_to_none(self):
         """Test that APMPackage type defaults to None when not provided."""
         package = APMPackage(name="test", version="1.0.0")
-        assert package.type is None
+        assert package.type is None
+
+
+class TestGenericHostSubdirectoryRoundTrip:
+    """Regression tests for issue #382: subdirectory packages on generic git hosts.
+
+    The str() → parse() round-trip must preserve virtual_path for all hosts,
+    not just GitHub and ADO.
+    """
+
+    @pytest.mark.parametrize("git_url,path,ref,desc", [
+        ("https://git.example.com/ai/grandpa-s-skills", "dist/brain-council", "master", "reporter case"),
+        ("https://gitlab.com/my-org/my-group/my-skills", "dist/skill-a", "main", "GitLab nested groups"),
+        ("https://gitea.example.com/org/repo", "prompts/helper", "v1.0", "Gitea simple"),
+        ("https://bitbucket.example.com/team/prompts", "agents/summarizer", "develop", "Bitbucket self-hosted"),
+    ])
+    def test_parse_from_dict_preserves_virtual_path(self, git_url, path, ref, desc):
+        """parse_from_dict correctly separates repo URL from subdirectory path."""
+        entry = {"git": git_url, "path": path, "ref": ref}
+        dep = DependencyReference.parse_from_dict(entry)
+        assert dep.virtual_path == path, f"Failed for {desc}"
+        assert dep.is_virtual is True, f"Failed for {desc}"
+
+    @pytest.mark.parametrize("git_url,path,ref", [
+        ("https://git.example.com/ai/grandpa-s-skills", "dist/brain-council", "master"),
+        ("https://gitlab.com/org/repo", "prompts/helper", "v1.0"),
+        ("https://bitbucket.example.com/team/prompts", "agents/summarizer", "develop"),
+    ])
+    def test_download_package_skips_parse_with_structured_dep(self, git_url, path, ref):
+        """download_package must skip DependencyReference.parse() when given
+        a structured object, avoiding the lossy round-trip."""
+        entry = {"git": git_url, "path": path, "ref": ref}
+        dep = DependencyReference.parse_from_dict(entry)
+
+        from apm_cli.deps.github_downloader import GitHubPackageDownloader
+        downloader = GitHubPackageDownloader()
+
+        # Monkey-patch DependencyReference.parse to detect if it's called
+        original_parse = DependencyReference.parse
+        parse_called = False
+        @classmethod
+        def tracking_parse(cls, s):
+            nonlocal parse_called
+            parse_called = True
+            return original_parse(s)
+
+        DependencyReference.parse = tracking_parse
+        try:
+            # download_package will fail on the actual clone, but the important
+            # thing is that it does NOT call parse() when given an object
+            downloader.download_package(dep, Path("/tmp/apm-test-nonexistent"))
+        except Exception:
+            pass  # Expected: clone will fail, but parse should not be called
+        finally:
+            DependencyReference.parse = original_parse
+
+        assert not parse_called, (
+            "DependencyReference.parse() was called when passing a structured "
+            "DependencyReference — the lossy round-trip was NOT avoided"
+        )
+
+    def test_github_round_trip_works(self):
+        """GitHub round-trip works because min_base_segments=2 is hardcoded."""
+        entry = {"git": "https://github.com/anthropics/skills", "path": "skills/skill-creator", "ref": "main"}
+        dep = DependencyReference.parse_from_dict(entry)
+        dep2 = DependencyReference.parse(str(dep))
+        assert dep2.virtual_path == dep.virtual_path
+        assert dep2.is_virtual == dep.is_virtual
+
+    def test_build_download_ref_preserves_virtual_path(self):
+        """build_download_ref returns a DependencyReference that preserves
+        virtual_path for generic hosts (not a lossy flat string)."""
+        from apm_cli.drift import build_download_ref
+        from unittest.mock import Mock
+
+        dep = DependencyReference(
+            repo_url="org/my-skills",
+            host="git.example.com",
+            reference="main",
+            virtual_path="dist/brain-council",
+            is_virtual=True,
+        )
+        lockfile = Mock()
+        locked_dep = Mock()
+        locked_dep.resolved_commit = "abc123"
+        lockfile.get_dependency = Mock(return_value=locked_dep)
+
+        result = build_download_ref(dep, lockfile, update_refs=False, ref_changed=False)
+        assert result.virtual_path == "dist/brain-council"
+        assert result.repo_url == "org/my-skills"
+        assert result.host == "git.example.com"
+        assert result.reference == "abc123"
+        assert result.is_virtual is True

tests/test_github_downloader.py

@@ -88,7 +88,7 @@ def test_resolve_git_reference_branch(self, mock_mkdtemp, mock_repo_class):
             result = self.downloader.resolve_git_reference('user/repo#main')
 
             assert isinstance(result, ResolvedReference)
-            assert result.original_ref == 'user/repo#main'
+            assert result.original_ref == 'github.com/user/repo#main'
             assert result.ref_type == GitReferenceType.BRANCH
             assert result.resolved_commit == 'abc123def456'
             assert result.ref_name == 'main'