bug: _parse_artifactory_base_url() ignores PROXY_REGISTRY_URL -- lockfile reinstall fails for virtual subdirectory packages

[chkp-roniz]

Bug

_parse_artifactory_base_url() in github_downloader.py (line 399) only reads the deprecated ARTIFACTORY_BASE_URL env var. It never checks the canonical PROXY_REGISTRY_URL introduced in #401.

This causes lockfile-based reinstalls of virtual subdirectory packages to fail when only PROXY_REGISTRY_URL is set.

Reproduction

# First install works (uses RegistryConfig.from_env() path)
export PROXY_REGISTRY_URL='https://art.example.com/artifactory/github'
apm marketplace add anthropics/skills
apm install arevlo-design@skills

# Lockfile correctly records:
#   host: art.example.com
#   registry_prefix: artifactory/github

# Clear installed files, reinstall from lockfile
rm -rf .github/prompts
apm install
# FAILS: git clone https://art.example.com/arevlo/claude-code-workflows (missing prefix)

Root Cause

The virtual subdirectory download path (download_package() line 1928) calls _parse_artifactory_base_url() to decide whether to use the Artifactory archive path:

elif dep_ref.is_virtual_subdirectory():
    art_proxy = self._parse_artifactory_base_url()  # only reads ARTIFACTORY_BASE_URL
    if self._is_artifactory_only() and art_proxy:
        return self._download_subdirectory_from_artifactory(...)
    return self.download_subdirectory_package(...)  # falls through to git clone

_parse_artifactory_base_url() returns None because it only checks ARTIFACTORY_BASE_URL:

def _parse_artifactory_base_url(self) -> Optional[tuple]:
    base_url = os.environ.get('ARTIFACTORY_BASE_URL', '').strip().rstrip('/')  # <-- ignores PROXY_REGISTRY_URL
    if not base_url:
        return None

Meanwhile, build_download_ref() in drift.py correctly restores host and artifactory_prefix on dep_ref from the lockfile, but the virtual subdirectory path never checks dep_ref.is_artifactory() -- unlike the non-virtual path (line 1939) which does.

Two Issues in One

Issue A: _parse_artifactory_base_url() doesn't read PROXY_REGISTRY_URL.

Issue B: The virtual subdirectory path doesn't handle "Mode 1: explicit FQDN from lockfile" (dep_ref.is_artifactory() check is missing). The non-virtual path at line 1939 handles it correctly:

# Non-virtual (correct -- checks both modes):
use_artifactory = dep_ref.is_artifactory()          # Mode 1: lockfile FQDN
if not use_artifactory:
    art_proxy = self._parse_artifactory_base_url()   # Mode 2: env var
    if art_proxy and self._should_use_artifactory_proxy(dep_ref):
        use_artifactory = True

# Virtual subdirectory (missing Mode 1):
art_proxy = self._parse_artifactory_base_url()       # Mode 2 only
if self._is_artifactory_only() and art_proxy:         # no dep_ref.is_artifactory() check

Suggested Fix

Fix A: Make _parse_artifactory_base_url() read canonical var first

def _parse_artifactory_base_url(self) -> Optional[tuple]:
    import urllib.parse as urlparse
    base_url = os.environ.get('PROXY_REGISTRY_URL', '').strip().rstrip('/')
    if not base_url:
        base_url = os.environ.get('ARTIFACTORY_BASE_URL', '').strip().rstrip('/')
    if not base_url:
        return None
    # ... rest unchanged

This aligns with how RegistryConfig.from_env() and is_enforce_only() already handle the canonical/deprecated var precedence.

Fix B: Add Mode 1 check to virtual subdirectory path

elif dep_ref.is_virtual_subdirectory():
    if dep_ref.is_artifactory():
        # Mode 1: lockfile restored FQDN + prefix
        return self._download_subdirectory_from_artifactory(
            dep_ref, target_path,
            (dep_ref.host, dep_ref.artifactory_prefix, "https"),
            progress_task_id, progress_obj,
        )
    art_proxy = self._parse_artifactory_base_url()
    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,
        )
    return self.download_subdirectory_package(dep_ref, target_path, progress_task_id, progress_obj)

Both fixes are needed: Fix A covers the env-var-only scenario, Fix B covers lockfile-based reinstalls where dep_ref already carries the Artifactory metadata.

Affected Versions

Present on main as of 81082e2 (v0.8.11). Introduced when PROXY_REGISTRY_URL was added in #401 but _parse_artifactory_base_url() was not updated.

Impact

• Users setting only PROXY_REGISTRY_URL (recommended canonical var) cannot reinstall virtual subdirectory packages from lockfile
• Users setting ARTIFACTORY_BASE_URL (deprecated) are unaffected
• Non-virtual packages and virtual file packages are unaffected (different code paths)

[sergio-sisternes-epam]

Thanks for the thorough report, @chkp-roniz — the root-cause analysis and dual-fix breakdown are excellent.

Triage summary:

• Confirmed regression: _parse_artifactory_base_url() was not updated when PROXY_REGISTRY_URL became the canonical env var in feat: lockfile-driven reproducible installs for registry proxies #401.
• Both sub-issues (A: env var precedence, B: missing Mode 1 check in virtual subdirectory path) are valid and need to be fixed together.
• PR fix: _parse_artifactory_base_url() ignores PROXY_REGISTRY_URL — lockfile reinstall fails (#614) #616 addresses both — we'll review it there.

Accepting this as bug / accepted. Thanks for the PR as well.