diff --git a/CHANGELOG.md b/CHANGELOG.md index 552745e73..848ac0689 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Propagate headers and environment variables through OpenCode MCP adapter with defensive copies to prevent mutation (#622) - Fix `apm install` hanging indefinitely when corporate firewalls silently drop SSH packets by setting `GIT_SSH_COMMAND` with `ConnectTimeout=30` (#652) - Fix `apm compile --target claude` silently skipping dependency instructions stored in `.github/instructions/` (#631) +- `_parse_artifactory_base_url()` now honours `PROXY_REGISTRY_URL` — lockfile reinstall no longer fails for virtual subdirectory packages (#614) ### Changed diff --git a/src/apm_cli/deps/github_downloader.py b/src/apm_cli/deps/github_downloader.py index f9887e6e4..93e44b7bf 100644 --- a/src/apm_cli/deps/github_downloader.py +++ b/src/apm_cli/deps/github_downloader.py @@ -412,20 +412,16 @@ def _should_use_artifactory_proxy(self, dep_ref: 'DependencyReference') -> bool: return is_github_hostname(host) def _parse_artifactory_base_url(self) -> Optional[tuple]: - """Parse ARTIFACTORY_BASE_URL into (host, prefix, scheme).""" - import urllib.parse as urlparse - base_url = os.environ.get('ARTIFACTORY_BASE_URL', '').strip().rstrip('/') - if not base_url: - return None - parsed = urlparse.urlparse(base_url) - if parsed.scheme not in ('https', 'http'): - _debug(f"ARTIFACTORY_BASE_URL has unsupported scheme: {parsed.scheme}") - return None - host = parsed.hostname - path = parsed.path.strip('/') - if not host or not path: + """Return ``(host, prefix, scheme)`` from the registry proxy config, or ``None``. + + Delegates to :meth:`~apm_cli.deps.registry_proxy.RegistryConfig.from_env` + so that env-var precedence and deprecation warnings are handled in one place. + """ + from .registry_proxy import RegistryConfig + cfg = RegistryConfig.from_env() + if cfg is None: return None - return (host, path, parsed.scheme) + return (cfg.host, cfg.prefix, cfg.scheme) def _resolve_dep_token(self, dep_ref: Optional[DependencyReference] = None) -> Optional[str]: """Resolve the per-dependency auth token via AuthResolver. @@ -2125,8 +2121,13 @@ def download_package( elif dep_ref.is_virtual_collection(): return self.download_collection_package(dep_ref, target_path, progress_task_id, progress_obj) elif dep_ref.is_virtual_subdirectory(): - # When PROXY_REGISTRY_ONLY is set, download full archive and extract subdir - art_proxy = self._parse_artifactory_base_url() + # Mode 1: explicit Artifactory FQDN from lockfile + if dep_ref.is_artifactory(): + proxy_info = (dep_ref.host, dep_ref.artifactory_prefix, "https") + return self._download_subdirectory_from_artifactory( + dep_ref, target_path, proxy_info, progress_task_id, progress_obj + ) + # Mode 2: transparent proxy via env var (art_proxy computed above) if self._is_artifactory_only() and art_proxy: return self._download_subdirectory_from_artifactory( dep_ref, target_path, art_proxy, progress_task_id, progress_obj diff --git a/tests/unit/test_artifactory_support.py b/tests/unit/test_artifactory_support.py index b32288552..fd0d212e3 100644 --- a/tests/unit/test_artifactory_support.py +++ b/tests/unit/test_artifactory_support.py @@ -389,10 +389,10 @@ def test_should_not_proxy_non_github_fqdn(self): assert not self.downloader._should_use_artifactory_proxy(dep) def test_parse_artifactory_base_url_valid(self): - """Parse valid ARTIFACTORY_BASE_URL.""" + """Parse valid PROXY_REGISTRY_URL.""" with patch.dict( os.environ, - {"ARTIFACTORY_BASE_URL": "https://art.example.com/artifactory/github"}, + {"PROXY_REGISTRY_URL": "https://art.example.com/artifactory/github"}, ): result = self.downloader._parse_artifactory_base_url() assert result is not None @@ -405,7 +405,7 @@ def test_parse_artifactory_base_url_trailing_slash(self): """Trailing slash in URL should be stripped.""" with patch.dict( os.environ, - {"ARTIFACTORY_BASE_URL": "https://art.example.com/artifactory/github/"}, + {"PROXY_REGISTRY_URL": "https://art.example.com/artifactory/github/"}, ): result = self.downloader._parse_artifactory_base_url() assert result is not None @@ -420,7 +420,7 @@ def test_parse_artifactory_base_url_not_set(self): def test_parse_artifactory_base_url_empty(self): """Returns None for empty string.""" - with patch.dict(os.environ, {"ARTIFACTORY_BASE_URL": ""}, clear=True): + with patch.dict(os.environ, {"PROXY_REGISTRY_URL": ""}, clear=True): result = self.downloader._parse_artifactory_base_url() assert result is None @@ -746,28 +746,28 @@ def test_oversized_archive_rejected(self): ) def test_parse_base_url_rejects_ftp_scheme(self): - """ARTIFACTORY_BASE_URL with non-http(s) scheme is rejected.""" + """PROXY_REGISTRY_URL with non-http(s) scheme is rejected.""" with patch.dict( os.environ, - {"ARTIFACTORY_BASE_URL": "ftp://art.example.com/artifactory/github"}, + {"PROXY_REGISTRY_URL": "ftp://art.example.com/artifactory/github"}, ): result = self.downloader._parse_artifactory_base_url() assert result is None def test_parse_base_url_rejects_no_scheme(self): - """ARTIFACTORY_BASE_URL without scheme is rejected.""" + """PROXY_REGISTRY_URL without scheme is rejected.""" with patch.dict( os.environ, - {"ARTIFACTORY_BASE_URL": "art.example.com/artifactory/github"}, + {"PROXY_REGISTRY_URL": "art.example.com/artifactory/github"}, ): result = self.downloader._parse_artifactory_base_url() assert result is None def test_parse_base_url_accepts_http(self): - """ARTIFACTORY_BASE_URL with http scheme is accepted (local dev).""" + """PROXY_REGISTRY_URL with http scheme is accepted (local dev).""" with patch.dict( os.environ, - {"ARTIFACTORY_BASE_URL": "http://localhost:8081/artifactory/github"}, + {"PROXY_REGISTRY_URL": "http://localhost:8081/artifactory/github"}, ): result = self.downloader._parse_artifactory_base_url() assert result is not None @@ -807,31 +807,31 @@ def test_no_corporate_values_in_source(self): ), f"Found forbidden term '{term}' in {py_file}" -# ── ARTIFACTORY_ONLY mode tests ── +# -- PROXY_REGISTRY_ONLY mode tests -- -class TestArtifactoryOnlyMode: - """Test ARTIFACTORY_ONLY env var blocking direct git operations.""" +class TestProxyRegistryOnlyMode: + """Test PROXY_REGISTRY_ONLY env var blocking direct git operations.""" def setup_method(self): self.downloader = GitHubPackageDownloader() def test_is_artifactory_only_flag(self): """_is_artifactory_only reads env var.""" - with patch.dict(os.environ, {"ARTIFACTORY_ONLY": "1"}): + with patch.dict(os.environ, {"PROXY_REGISTRY_ONLY": "1"}): assert GitHubPackageDownloader._is_artifactory_only() - with patch.dict(os.environ, {"ARTIFACTORY_ONLY": "true"}): + with patch.dict(os.environ, {"PROXY_REGISTRY_ONLY": "true"}): assert GitHubPackageDownloader._is_artifactory_only() - with patch.dict(os.environ, {"ARTIFACTORY_ONLY": "yes"}): + with patch.dict(os.environ, {"PROXY_REGISTRY_ONLY": "yes"}): assert GitHubPackageDownloader._is_artifactory_only() - with patch.dict(os.environ, {"ARTIFACTORY_ONLY": ""}): + with patch.dict(os.environ, {"PROXY_REGISTRY_ONLY": ""}): assert not GitHubPackageDownloader._is_artifactory_only() with patch.dict(os.environ, {}, clear=True): assert not GitHubPackageDownloader._is_artifactory_only() def test_proxy_routes_all_when_artifactory_only(self): - """ARTIFACTORY_ONLY makes _should_use_artifactory_proxy return True for all non-Artifactory deps.""" - with patch.dict(os.environ, {"ARTIFACTORY_ONLY": "1"}): + """PROXY_REGISTRY_ONLY makes _should_use_artifactory_proxy return True for all non-Artifactory deps.""" + with patch.dict(os.environ, {"PROXY_REGISTRY_ONLY": "1"}): # GitHub dep dep = DependencyReference.parse("microsoft/apm-sample-package") assert self.downloader._should_use_artifactory_proxy(dep) @@ -843,16 +843,16 @@ def test_proxy_routes_all_when_artifactory_only(self): assert self.downloader._should_use_artifactory_proxy(dep) def test_proxy_still_skips_explicit_artifactory(self): - """Already-Artifactory deps should not be double-proxied even with ARTIFACTORY_ONLY.""" - with patch.dict(os.environ, {"ARTIFACTORY_ONLY": "1"}): + """Already-Artifactory deps should not be double-proxied even with PROXY_REGISTRY_ONLY.""" + with patch.dict(os.environ, {"PROXY_REGISTRY_ONLY": "1"}): dep = DependencyReference.parse("art.example.com/artifactory/github/owner/repo") assert not self.downloader._should_use_artifactory_proxy(dep) def test_resolve_ref_skips_git_when_artifactory_only(self): - """resolve_git_reference skips git for all deps when ARTIFACTORY_ONLY is set.""" + """resolve_git_reference skips git for all deps when PROXY_REGISTRY_ONLY is set.""" with patch.dict(os.environ, { - "ARTIFACTORY_ONLY": "1", - "ARTIFACTORY_BASE_URL": "https://art.example.com/artifactory/github", + "PROXY_REGISTRY_ONLY": "1", + "PROXY_REGISTRY_URL": "https://art.example.com/artifactory/github", }): dl = GitHubPackageDownloader() ref = dl.resolve_git_reference("gitlab.com/owner/repo#develop") @@ -860,15 +860,15 @@ def test_resolve_ref_skips_git_when_artifactory_only(self): assert ref.resolved_commit is None def test_download_package_errors_without_base_url(self): - """ARTIFACTORY_ONLY without ARTIFACTORY_BASE_URL raises for non-Artifactory deps.""" - with patch.dict(os.environ, {"ARTIFACTORY_ONLY": "1"}, clear=True): + """PROXY_REGISTRY_ONLY without PROXY_REGISTRY_URL raises for non-proxy deps.""" + with patch.dict(os.environ, {"PROXY_REGISTRY_ONLY": "1"}, clear=True): dl = GitHubPackageDownloader() with pytest.raises(RuntimeError, match="PROXY_REGISTRY_ONLY is set"): dl.download_package("microsoft/some-package", Path("/tmp/test-pkg")) def test_virtual_file_errors_without_base_url(self): - """ARTIFACTORY_ONLY without ARTIFACTORY_BASE_URL raises for virtual file packages.""" - with patch.dict(os.environ, {"ARTIFACTORY_ONLY": "1"}, clear=True): + """PROXY_REGISTRY_ONLY without PROXY_REGISTRY_URL raises for virtual file packages.""" + with patch.dict(os.environ, {"PROXY_REGISTRY_ONLY": "1"}, clear=True): dl = GitHubPackageDownloader() with pytest.raises(RuntimeError, match="PROXY_REGISTRY_ONLY is set"): dl.download_package( @@ -876,8 +876,8 @@ def test_virtual_file_errors_without_base_url(self): ) def test_virtual_collection_errors_without_base_url(self): - """ARTIFACTORY_ONLY without ARTIFACTORY_BASE_URL raises for virtual collection packages.""" - with patch.dict(os.environ, {"ARTIFACTORY_ONLY": "1"}, clear=True): + """PROXY_REGISTRY_ONLY without PROXY_REGISTRY_URL raises for virtual collection packages.""" + with patch.dict(os.environ, {"PROXY_REGISTRY_ONLY": "1"}, clear=True): dl = GitHubPackageDownloader() with pytest.raises(RuntimeError, match="PROXY_REGISTRY_ONLY is set"): dl.download_package( @@ -885,8 +885,8 @@ def test_virtual_collection_errors_without_base_url(self): ) def test_virtual_subdirectory_errors_without_base_url(self): - """ARTIFACTORY_ONLY without ARTIFACTORY_BASE_URL raises for virtual subdirectory packages.""" - with patch.dict(os.environ, {"ARTIFACTORY_ONLY": "1"}, clear=True): + """PROXY_REGISTRY_ONLY without PROXY_REGISTRY_URL raises for virtual subdirectory packages.""" + with patch.dict(os.environ, {"PROXY_REGISTRY_ONLY": "1"}, clear=True): dl = GitHubPackageDownloader() with pytest.raises(RuntimeError, match="PROXY_REGISTRY_ONLY is set"): dl.download_package( @@ -894,8 +894,8 @@ def test_virtual_subdirectory_errors_without_base_url(self): ) def test_explicit_artifactory_fqdn_virtual_file_passes(self): - """Explicit Artifactory FQDN on virtual file dep is NOT blocked by ARTIFACTORY_ONLY.""" - with patch.dict(os.environ, {"ARTIFACTORY_ONLY": "1"}, clear=True): + """Explicit Artifactory FQDN on virtual file dep is NOT blocked by PROXY_REGISTRY_ONLY.""" + with patch.dict(os.environ, {"PROXY_REGISTRY_ONLY": "1"}, clear=True): dl = GitHubPackageDownloader() dep = DependencyReference.parse( "art.example.com/artifactory/github/owner/repo/prompts/deploy.prompt.md" @@ -907,8 +907,8 @@ def test_explicit_artifactory_fqdn_virtual_file_passes(self): dl.download_package(dep, Path("/tmp/test-pkg")) def test_explicit_artifactory_fqdn_virtual_collection_passes(self): - """Explicit Artifactory FQDN on virtual collection dep is NOT blocked by ARTIFACTORY_ONLY.""" - with patch.dict(os.environ, {"ARTIFACTORY_ONLY": "1"}, clear=True): + """Explicit Artifactory FQDN on virtual collection dep is NOT blocked by PROXY_REGISTRY_ONLY.""" + with patch.dict(os.environ, {"PROXY_REGISTRY_ONLY": "1"}, clear=True): dl = GitHubPackageDownloader() dep = DependencyReference.parse( "art.example.com/artifactory/github/owner/repo/collections/my-collection" @@ -919,8 +919,8 @@ def test_explicit_artifactory_fqdn_virtual_collection_passes(self): with patch.object(dl, 'download_collection_package', return_value=MagicMock()): dl.download_package(dep, Path("/tmp/test-pkg")) - def test_proxy_registry_only_raises_same_as_artifactory_only(self): - """PROXY_REGISTRY_ONLY=1 is the canonical name and also raises for non-proxy deps.""" + def test_proxy_registry_only_is_canonical(self): + """PROXY_REGISTRY_ONLY=1 is the canonical name and raises for non-proxy deps.""" with patch.dict(os.environ, {"PROXY_REGISTRY_ONLY": "1"}, clear=True): dl = GitHubPackageDownloader() with pytest.raises(RuntimeError, match="PROXY_REGISTRY_ONLY is set"): @@ -1567,3 +1567,156 @@ def test_entry_download_with_no_headers(self): assert result == b"public" assert mock_get.call_args[1]["headers"] == {} + + +# -- Fix A: _parse_artifactory_base_url reads PROXY_REGISTRY_URL -- + + +class TestParseArtifactoryBaseUrlCanonicalVar: + """_parse_artifactory_base_url reads PROXY_REGISTRY_URL first, falls back to ARTIFACTORY_BASE_URL.""" + + def setup_method(self): + self.downloader = GitHubPackageDownloader() + + def test_proxy_registry_url_is_preferred(self): + """PROXY_REGISTRY_URL takes precedence over ARTIFACTORY_BASE_URL.""" + with patch.dict( + os.environ, + { + "PROXY_REGISTRY_URL": "https://proxy.example.com/registry/github", + "ARTIFACTORY_BASE_URL": "https://art.example.com/artifactory/github", + }, + clear=True, + ): + result = self.downloader._parse_artifactory_base_url() + assert result is not None + host, prefix, scheme = result + assert host == "proxy.example.com" + assert prefix == "registry/github" + + def test_proxy_registry_url_alone(self): + """PROXY_REGISTRY_URL works when ARTIFACTORY_BASE_URL is not set.""" + with patch.dict( + os.environ, + {"PROXY_REGISTRY_URL": "https://art.example.com/artifactory/github"}, + clear=True, + ): + result = self.downloader._parse_artifactory_base_url() + assert result is not None + host, prefix, scheme = result + assert host == "art.example.com" + assert prefix == "artifactory/github" + assert scheme == "https" + + def test_falls_back_to_deprecated_var(self): + """Falls back to ARTIFACTORY_BASE_URL when PROXY_REGISTRY_URL is absent.""" + with patch.dict( + os.environ, + {"ARTIFACTORY_BASE_URL": "https://art.example.com/artifactory/github"}, + clear=True, + ): + result = self.downloader._parse_artifactory_base_url() + assert result is not None + assert result[0] == "art.example.com" + + def test_deprecated_var_emits_warning(self): + """Falling back to ARTIFACTORY_BASE_URL emits DeprecationWarning.""" + import warnings + + with patch.dict( + os.environ, + {"ARTIFACTORY_BASE_URL": "https://art.example.com/artifactory/github"}, + clear=True, + ): + with warnings.catch_warnings(record=True) as w: + warnings.simplefilter("always") + self.downloader._parse_artifactory_base_url() + assert any(issubclass(x.category, DeprecationWarning) for x in w) + assert any("ARTIFACTORY_BASE_URL" in str(x.message) for x in w) + + def test_neither_var_set(self): + """Returns None when neither env var is set.""" + with patch.dict(os.environ, {}, clear=True): + result = self.downloader._parse_artifactory_base_url() + assert result is None + + +# -- Fix B: virtual subdirectory uses lockfile FQDN (Mode 1) -- + + +class TestVirtualSubdirectoryLockfileReinstall: + """Virtual subdirectory packages use lockfile FQDN metadata directly.""" + + def setup_method(self): + self.downloader = GitHubPackageDownloader() + + def test_subdirectory_uses_lockfile_fqdn(self): + """When dep_ref.is_artifactory(), subdirectory download uses FQDN, not env var.""" + dep = DependencyReference.parse( + "art.example.com/artifactory/github/owner/repo//subdir" + ) + assert dep.is_artifactory() + assert dep.is_virtual_subdirectory() + + target = Path("/tmp/test-subdir") + with patch.object( + self.downloader, + "_download_subdirectory_from_artifactory", + return_value=Mock(), + ) as mock_dl: + self.downloader.download_package(dep, target) + mock_dl.assert_called_once() + proxy_info = mock_dl.call_args[0][2] + assert proxy_info[0] == "art.example.com" + assert proxy_info[1] == "artifactory/github" + + def test_subdirectory_fqdn_no_env_var_needed(self): + """Lockfile FQDN path works without any env var set.""" + dep = DependencyReference.parse( + "art.example.com/artifactory/github/owner/repo//subdir" + ) + target = Path("/tmp/test-subdir") + with patch.dict(os.environ, {}, clear=True): + with patch.object( + self.downloader, + "_download_subdirectory_from_artifactory", + return_value=Mock(), + ) as mock_dl: + self.downloader.download_package(dep, target) + mock_dl.assert_called_once() + + def test_subdirectory_fqdn_takes_precedence_over_only_mode(self): + """Mode 1 FQDN takes precedence even when PROXY_REGISTRY_ONLY is set.""" + dep = DependencyReference.parse( + "art.example.com/artifactory/github/owner/repo//subdir" + ) + target = Path("/tmp/test-subdir") + with patch.dict(os.environ, {"PROXY_REGISTRY_ONLY": "1"}, clear=True): + with patch.object( + self.downloader, + "_download_subdirectory_from_artifactory", + return_value=Mock(), + ) as mock_dl: + self.downloader.download_package(dep, target) + mock_dl.assert_called_once() + proxy_info = mock_dl.call_args[0][2] + assert proxy_info[0] == "art.example.com" + + +# -- Backward compat: deprecated ARTIFACTORY_ONLY still works -- + + +class TestDeprecatedArtifactoryOnlyBackwardCompat: + """ARTIFACTORY_ONLY (deprecated) still works through _is_artifactory_only().""" + + def test_artifactory_only_still_triggers_enforce(self): + """Deprecated ARTIFACTORY_ONLY=1 still activates enforce-only mode.""" + with patch.dict(os.environ, {"ARTIFACTORY_ONLY": "1"}, clear=True): + assert GitHubPackageDownloader._is_artifactory_only() + + def test_artifactory_only_still_blocks_direct_git(self): + """Deprecated ARTIFACTORY_ONLY=1 still blocks direct git downloads.""" + with patch.dict(os.environ, {"ARTIFACTORY_ONLY": "1"}, clear=True): + dl = GitHubPackageDownloader() + with pytest.raises(RuntimeError, match="PROXY_REGISTRY_ONLY is set"): + dl.download_package("microsoft/some-package", Path("/tmp/test-pkg"))