Merge pull request #178 from sergio-sisternes-epam/fix/177-lockfile-commit-sha-clone

fix: handle commit SHA refs in subdirectory package clones

Author: sergio-sisternes-epam

src/apm_cli/deps/github_downloader.py

@@ -1138,19 +1138,27 @@ def download_subdirectory_package(self, dep_ref: DependencyReference, target_pat
             sparse_ok = self._try_sparse_checkout(dep_ref, temp_clone_path, subdir_path, ref)
 
             if not sparse_ok:
-                # Full shallow clone fallback
+                # Full clone fallback
                 if temp_clone_path.exists():
                     shutil.rmtree(temp_clone_path)
 
                 package_display_name = subdir_path.split('/')[-1]
                 progress_reporter = GitProgressReporter(progress_task_id, progress_obj, package_display_name) if progress_task_id and progress_obj else None
 
+                # Detect if ref is a commit SHA (can't be used with --branch in shallow clones)
+                is_commit_sha = ref and re.match(r'^[a-f0-9]{7,40}$', ref) is not None
+                
                 clone_kwargs = {
                     'dep_ref': dep_ref,
-                    'depth': 1,
                 }
-                if ref:
-                    clone_kwargs['branch'] = ref
+                if is_commit_sha:
+                    # For commit SHAs, clone without checkout then checkout the specific commit.
+                    # Shallow clone doesn't support fetching by arbitrary SHA.
+                    clone_kwargs['no_checkout'] = True
+                else:
+                    clone_kwargs['depth'] = 1
+                    if ref:
+                        clone_kwargs['branch'] = ref
 
                 try:
                     self._clone_with_fallback(
@@ -1160,7 +1168,14 @@ def download_subdirectory_package(self, dep_ref: DependencyReference, target_pat
                         **clone_kwargs
                     )
                 except Exception as e:
-                    raise RuntimeError(f"Failed to clone repository: {e}")
+                    raise RuntimeError(f"Failed to clone repository: {e}") from e
+                
+                if is_commit_sha:
+                    try:
+                        repo_obj = Repo(temp_clone_path)
+                        repo_obj.git.checkout(ref)
+                    except Exception as e:
+                        raise RuntimeError(f"Failed to checkout commit {ref}: {e}") from e
 
                 # Disable progress reporter after clone
                 if progress_reporter:

tests/test_github_downloader.py

@@ -828,5 +828,175 @@ def test_ghe_without_github_token_falls_back(self):
             assert 'ado-token' not in url
 
 
+class TestSubdirectoryPackageCommitSHA:
+    """Test commit SHA handling in download_subdirectory_package."""
+
+    def setup_method(self):
+        self.temp_dir = Path(tempfile.mkdtemp())
+
+    def teardown_method(self):
+        if self.temp_dir.exists():
+            shutil.rmtree(self.temp_dir, ignore_errors=True)
+
+    def _make_dep_ref(self, ref=None):
+        """Create a virtual subdirectory DependencyReference."""
+        dep = DependencyReference(
+            repo_url='owner/monorepo',
+            host='github.com',
+            reference=ref,
+            virtual_path='packages/my-skill',
+            is_virtual=True,
+        )
+        return dep
+
+    @patch('apm_cli.deps.github_downloader.Repo')
+    @patch('apm_cli.deps.github_downloader.validate_apm_package')
+    def test_sha_ref_clones_without_depth_and_checks_out(self, mock_validate, mock_repo_class):
+        """Commit SHA refs must clone with no_checkout (no depth/branch) then checkout the SHA."""
+        sha = 'a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2'
+        dep_ref = self._make_dep_ref(ref=sha)
+
+        mock_repo = Mock()
+        mock_repo_class.return_value = mock_repo
+
+        mock_validation = ValidationResult()
+        mock_validation.is_valid = True
+        mock_validation.package = APMPackage(name='my-skill', version='1.0.0')
+        mock_validate.return_value = mock_validation
+
+        with patch.dict(os.environ, {}, clear=True):
+            downloader = GitHubPackageDownloader()
+
+        with patch.object(downloader, '_clone_with_fallback') as mock_clone:
+            mock_clone.return_value = mock_repo
+
+            target = self.temp_dir / 'my-skill'
+
+            # Create the subdirectory structure that download_subdirectory_package expects
+            def setup_subdir(*args, **kwargs):
+                clone_path = args[1]
+                subdir = clone_path / 'packages' / 'my-skill'
+                subdir.mkdir(parents=True)
+                (subdir / 'apm.yml').write_text('name: my-skill\nversion: 1.0.0\n')
+                return mock_repo
+
+            mock_clone.side_effect = setup_subdir
+
+            downloader.download_subdirectory_package(dep_ref, target)
+
+            # Verify clone was called without depth/branch but WITH no_checkout
+            call_kwargs = mock_clone.call_args
+            assert 'depth' not in call_kwargs.kwargs, "SHA ref should NOT use shallow clone"
+            assert 'branch' not in call_kwargs.kwargs, "SHA ref should NOT pass branch"
+            assert call_kwargs.kwargs.get('no_checkout') is True, "SHA ref should use no_checkout=True"
+
+            # Verify checkout was called with the SHA
+            mock_repo.git.checkout.assert_called_once_with(sha)
+
+    @patch('apm_cli.deps.github_downloader.Repo')
+    @patch('apm_cli.deps.github_downloader.validate_apm_package')
+    def test_branch_ref_uses_shallow_clone(self, mock_validate, mock_repo_class):
+        """Branch/tag refs must use shallow clone with depth=1 and branch kwarg."""
+        dep_ref = self._make_dep_ref(ref='main')
+
+        mock_repo = Mock()
+        mock_repo_class.return_value = mock_repo
+
+        mock_validation = ValidationResult()
+        mock_validation.is_valid = True
+        mock_validation.package = APMPackage(name='my-skill', version='1.0.0')
+        mock_validate.return_value = mock_validation
+
+        with patch.dict(os.environ, {}, clear=True):
+            downloader = GitHubPackageDownloader()
+
+        with patch.object(downloader, '_clone_with_fallback') as mock_clone:
+            mock_clone.return_value = mock_repo
+
+            target = self.temp_dir / 'my-skill'
+
+            def setup_subdir(*args, **kwargs):
+                clone_path = args[1]
+                subdir = clone_path / 'packages' / 'my-skill'
+                subdir.mkdir(parents=True)
+                (subdir / 'apm.yml').write_text('name: my-skill\nversion: 1.0.0\n')
+                return mock_repo
+
+            mock_clone.side_effect = setup_subdir
+
+            downloader.download_subdirectory_package(dep_ref, target)
+
+            call_kwargs = mock_clone.call_args
+            assert call_kwargs.kwargs.get('depth') == 1, "Branch ref should use depth=1"
+            assert call_kwargs.kwargs.get('branch') == 'main', "Branch ref should pass branch"
+            assert 'no_checkout' not in call_kwargs.kwargs, "Branch ref should not set no_checkout"
+
+            # No explicit checkout for branch refs
+            mock_repo.git.checkout.assert_not_called()
+
+    @patch('apm_cli.deps.github_downloader.Repo')
+    @patch('apm_cli.deps.github_downloader.validate_apm_package')
+    def test_no_ref_uses_shallow_clone_without_branch(self, mock_validate, mock_repo_class):
+        """No ref should use shallow clone without branch kwarg (default branch)."""
+        dep_ref = self._make_dep_ref(ref=None)
+
+        mock_repo = Mock()
+        mock_repo_class.return_value = mock_repo
+
+        mock_validation = ValidationResult()
+        mock_validation.is_valid = True
+        mock_validation.package = APMPackage(name='my-skill', version='1.0.0')
+        mock_validate.return_value = mock_validation
+
+        with patch.dict(os.environ, {}, clear=True):
+            downloader = GitHubPackageDownloader()
+
+        with patch.object(downloader, '_clone_with_fallback') as mock_clone:
+            mock_clone.return_value = mock_repo
+
+            target = self.temp_dir / 'my-skill'
+
+            def setup_subdir(*args, **kwargs):
+                clone_path = args[1]
+                subdir = clone_path / 'packages' / 'my-skill'
+                subdir.mkdir(parents=True)
+                (subdir / 'apm.yml').write_text('name: my-skill\nversion: 1.0.0\n')
+                return mock_repo
+
+            mock_clone.side_effect = setup_subdir
+
+            downloader.download_subdirectory_package(dep_ref, target)
+
+            call_kwargs = mock_clone.call_args
+            assert call_kwargs.kwargs.get('depth') == 1, "No ref should still shallow clone"
+            assert 'branch' not in call_kwargs.kwargs, "No ref should not pass branch"
+
+    @patch('apm_cli.deps.github_downloader.Repo')
+    def test_sha_checkout_failure_raises_descriptive_error(self, mock_repo_class):
+        """Checkout failure for SHA ref should raise error mentioning 'checkout', not 'clone'."""
+        sha = 'a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2'
+        dep_ref = self._make_dep_ref(ref=sha)
+
+        mock_repo = Mock()
+        mock_repo.git.checkout.side_effect = Exception("bad object a1b2c3d")
+        mock_repo_class.return_value = mock_repo
+
+        with patch.dict(os.environ, {}, clear=True):
+            downloader = GitHubPackageDownloader()
+
+        with patch.object(downloader, '_clone_with_fallback') as mock_clone:
+            def setup_subdir(*args, **kwargs):
+                clone_path = args[1]
+                subdir = clone_path / 'packages' / 'my-skill'
+                subdir.mkdir(parents=True)
+                return mock_repo
+
+            mock_clone.side_effect = setup_subdir
+
+            target = self.temp_dir / 'my-skill'
+            with pytest.raises(RuntimeError, match="Failed to checkout commit"):
+                downloader.download_subdirectory_package(dep_ref, target)
+
+
 if __name__ == '__main__':
     pytest.main([__file__])