From f804b0e11e9cc2dec3dd902fe77203cf855baf9d Mon Sep 17 00:00:00 2001 From: sergio-sisternes-epam Date: Fri, 6 Mar 2026 00:33:32 +0000 Subject: [PATCH 1/2] fix: handle commit SHA refs in subdirectory package clones When a lockfile contains a resolved commit SHA, download_subdirectory_package was passing it as --branch to a shallow clone, which git does not support. Now detects commit SHAs and uses a full clone + checkout instead. Fixes #177 --- src/apm_cli/deps/github_downloader.py | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/src/apm_cli/deps/github_downloader.py b/src/apm_cli/deps/github_downloader.py index 13189a34..90a8a740 100644 --- a/src/apm_cli/deps/github_downloader.py +++ b/src/apm_cli/deps/github_downloader.py @@ -1139,19 +1139,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( @@ -1161,7 +1169,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: From eff6a6e80309415f86aaa46b03b044378a8e94b8 Mon Sep 17 00:00:00 2001 From: sergio-sisternes-epam Date: Fri, 6 Mar 2026 08:53:55 +0000 Subject: [PATCH 2/2] fix: address review feedback on commit SHA clone handling - Use no_checkout=True instead of full working-tree clone for SHA refs - Separate try/except for clone vs checkout with descriptive error messages - Use exception chaining (raise ... from e) to preserve tracebacks - Remove unnecessary ref.lower() call - Add 4 unit tests covering SHA, branch, no-ref, and checkout failure paths --- tests/test_github_downloader.py | 170 ++++++++++++++++++++++++++++++++ 1 file changed, 170 insertions(+) diff --git a/tests/test_github_downloader.py b/tests/test_github_downloader.py index 0082d70b..67d634f0 100644 --- a/tests/test_github_downloader.py +++ b/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__]) \ No newline at end of file