From c1b66bd5c83f738707fe0aa67013428be3791ffe Mon Sep 17 00:00:00 2001 From: "ENABLON\\thomas.caudal" Date: Mon, 23 Mar 2026 14:42:31 +0100 Subject: [PATCH 01/10] fix: share AuthResolver across install to prevent duplicate auth popups Create a single AuthResolver at the top of the install command and thread it through _validate_and_add_packages_to_apm_yml, _validate_package_exists, and _install_apm_dependencies so that all validation and download steps within one CLI invocation share the same credential cache. Also fixes a lock inversion in AuthResolver.resolve() that allowed two concurrent callers for the same (host, org) key to each trigger their own credential-helper lookup before either result was cached. --- dist.old/apm-windows-x86_64.sha256 | 1 + src/apm_cli/commands/install.py | 41 ++++++++++++++++++--------- src/apm_cli/core/auth.py | 35 ++++++++++++----------- src/apm_cli/deps/github_downloader.py | 27 +++++++++--------- tests/test_github_downloader.py | 9 ++++++ tests/unit/test_auth.py | 21 ++++++++++++++ 6 files changed, 90 insertions(+), 44 deletions(-) create mode 100644 dist.old/apm-windows-x86_64.sha256 diff --git a/dist.old/apm-windows-x86_64.sha256 b/dist.old/apm-windows-x86_64.sha256 new file mode 100644 index 000000000..bba469bf6 --- /dev/null +++ b/dist.old/apm-windows-x86_64.sha256 @@ -0,0 +1 @@ +56b2be37fde6ccdef0074d696463980768078cf5ff39757e2837a7e05c6ff581 dist/apm-windows-x86_64/apm.exe diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index e7a0ca730..7d1c39a1f 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -39,6 +39,7 @@ APM_DEPS_AVAILABLE = False _APM_IMPORT_ERROR = None try: + from ..core.auth import AuthResolver from ..deps.apm_resolver import APMDependencyResolver from ..deps.github_downloader import GitHubPackageDownloader from ..deps.lockfile import LockFile, get_lockfile_path, migrate_lockfile_if_needed @@ -56,7 +57,7 @@ # --------------------------------------------------------------------------- -def _validate_and_add_packages_to_apm_yml(packages, dry_run=False, dev=False, logger=None): +def _validate_and_add_packages_to_apm_yml(packages, dry_run=False, dev=False, logger=None, auth_resolver=None): """Validate packages exist and can be accessed, then add to apm.yml dependencies section. Implements normalize-on-write: any input form (HTTPS URL, SSH URL, FQDN, shorthand) @@ -68,6 +69,7 @@ def _validate_and_add_packages_to_apm_yml(packages, dry_run=False, dev=False, lo dry_run: If True, only show what would be added. dev: If True, write to devDependencies instead of dependencies. logger: InstallLogger for structured output. + auth_resolver: Shared auth resolver for caching credentials. Returns: Tuple of (validated_packages list, _ValidationOutcome). @@ -148,7 +150,7 @@ def _validate_and_add_packages_to_apm_yml(packages, dry_run=False, dev=False, lo # Validate package exists and is accessible verbose = bool(logger and logger.verbose) - if _validate_package_exists(package, verbose=verbose): + if _validate_package_exists(package, verbose=verbose, auth_resolver=auth_resolver): valid_outcomes.append((canonical, already_in_deps)) if logger: logger.validation_pass(canonical, already_present=already_in_deps) @@ -214,7 +216,7 @@ def _validate_and_add_packages_to_apm_yml(packages, dry_run=False, dev=False, lo return validated_packages, outcome -def _validate_package_exists(package, verbose=False): +def _validate_package_exists(package, verbose=False, auth_resolver=None): """Validate that a package exists and is accessible on GitHub, Azure DevOps, or locally.""" import os import subprocess @@ -222,7 +224,9 @@ def _validate_package_exists(package, verbose=False): from apm_cli.core.auth import AuthResolver verbose_log = (lambda msg: _rich_echo(f" {msg}", color="dim")) if verbose else None - auth_resolver = AuthResolver() + # Use provided resolver or create new one if not in a CLI session context + if auth_resolver is None: + auth_resolver = AuthResolver() try: # Parse the package to check if it's a virtual package or ADO @@ -252,8 +256,8 @@ def _validate_package_exists(package, verbose=False): org = dep_ref.repo_url.split('/')[0] if dep_ref.repo_url and '/' in dep_ref.repo_url else None if verbose_log: verbose_log(f"Auth resolved: host={host}, org={org}, source={ctx.source}, type={ctx.token_type}") - downloader = GitHubPackageDownloader(auth_resolver=auth_resolver) - result = downloader.validate_virtual_package_exists(dep_ref) + virtual_downloader = GitHubPackageDownloader(auth_resolver=auth_resolver) + result = virtual_downloader.validate_virtual_package_exists(dep_ref) if not result and verbose_log: try: err_ctx = auth_resolver.build_error_context(host, f"accessing {package}", org=org) @@ -268,13 +272,13 @@ def _validate_package_exists(package, verbose=False): if dep_ref.is_azure_devops() or (dep_ref.host and dep_ref.host != "github.com"): from apm_cli.utils.github_host import is_github_hostname, is_azure_devops_hostname - downloader = GitHubPackageDownloader() + ado_downloader = GitHubPackageDownloader(auth_resolver=auth_resolver) # Set the host if dep_ref.host: - downloader.github_host = dep_ref.host + ado_downloader.github_host = dep_ref.host # Build authenticated URL using downloader's auth - package_url = downloader._build_repo_url( + package_url = ado_downloader._build_repo_url( dep_ref.repo_url, use_ssh=False, dep_ref=dep_ref ) @@ -283,11 +287,11 @@ def _validate_package_exists(package, verbose=False): # This mirrors _clone_with_fallback() which does the same relaxation. is_generic = not is_github_hostname(dep_ref.host) and not is_azure_devops_hostname(dep_ref.host) if is_generic: - validate_env = {k: v for k, v in downloader.git_env.items() + validate_env = {k: v for k, v in ado_downloader.git_env.items() if k not in ('GIT_ASKPASS', 'GIT_CONFIG_GLOBAL', 'GIT_CONFIG_NOSYSTEM')} validate_env['GIT_TERMINAL_PROMPT'] = '0' else: - validate_env = {**os.environ, **downloader.git_env} + validate_env = {**os.environ, **ado_downloader.git_env} if verbose_log: verbose_log(f"Trying git ls-remote for {dep_ref.host}") @@ -491,6 +495,10 @@ def install(ctx, packages, runtime, exclude, only, update, dry_run, force, verbo is_partial = bool(packages) logger = InstallLogger(verbose=verbose, dry_run=dry_run, partial=is_partial) + # Create shared auth resolver for all downloads in this CLI invocation + # to ensure credentials are cached and reused (prevents duplicate auth popups) + auth_resolver = AuthResolver() + # Check if apm.yml exists apm_yml_exists = Path(APM_YML_FILENAME).exists() @@ -512,7 +520,7 @@ def install(ctx, packages, runtime, exclude, only, update, dry_run, force, verbo # If packages are specified, validate and add them to apm.yml first if packages: validated_packages, outcome = _validate_and_add_packages_to_apm_yml( - packages, dry_run, dev=dev, logger=logger, + packages, dry_run, dev=dev, logger=logger, auth_resolver=auth_resolver, ) # Short-circuit: all packages failed validation — nothing to install if outcome.all_failed: @@ -613,6 +621,7 @@ def install(ctx, packages, runtime, exclude, only, update, dry_run, force, verbo apm_package, update, verbose, only_pkgs, force=force, parallel_downloads=parallel_downloads, logger=logger, + auth_resolver=auth_resolver, ) apm_count = install_result.installed_count prompt_count = install_result.prompts_integrated @@ -1023,6 +1032,7 @@ def _install_apm_dependencies( force: bool = False, parallel_downloads: int = 4, logger: "InstallLogger" = None, + auth_resolver: "AuthResolver" = None, ): """Install APM package dependencies. @@ -1034,6 +1044,7 @@ def _install_apm_dependencies( force: Whether to overwrite locally-authored files on collision parallel_downloads: Max concurrent downloads (0 disables parallelism) logger: InstallLogger for structured output + auth_resolver: Shared auth resolver for caching credentials """ if not APM_DEPS_AVAILABLE: raise RuntimeError("APM dependency system not available") @@ -1066,8 +1077,12 @@ def _install_apm_dependencies( apm_modules_dir = project_root / APM_MODULES_DIR apm_modules_dir.mkdir(exist_ok=True) + # Use provided resolver or create new one if not in a CLI session context + if auth_resolver is None: + auth_resolver = AuthResolver() + # Create downloader early so it can be used for transitive dependency resolution - downloader = GitHubPackageDownloader() + downloader = GitHubPackageDownloader(auth_resolver=auth_resolver) # Track direct dependency keys so the download callback can distinguish them from transitive direct_dep_keys = builtins.set(dep.get_unique_key() for dep in apm_deps) diff --git a/src/apm_cli/core/auth.py b/src/apm_cli/core/auth.py index e1e56ad77..dc569e5bf 100644 --- a/src/apm_cli/core/auth.py +++ b/src/apm_cli/core/auth.py @@ -182,25 +182,26 @@ def resolve(self, host: str, org: Optional[str] = None) -> AuthContext: """Resolve auth for *(host, org)*. Cached & thread-safe.""" key = (host.lower() if host else host, org.lower() if org else org) with self._lock: - if key in self._cache: - return self._cache[key] - - host_info = self.classify_host(host) - token, source = self._resolve_token(host_info, org) - token_type = self.detect_token_type(token) if token else "unknown" - git_env = self._build_git_env(token) - - ctx = AuthContext( - token=token, - source=source, - token_type=token_type, - host_info=host_info, - git_env=git_env, - ) + cached = self._cache.get(key) + if cached is not None: + return cached - with self._lock: + # Keep cache fill inside the lock to avoid concurrent duplicate + # credential-helper lookups for the same host/org. + host_info = self.classify_host(host) + token, source = self._resolve_token(host_info, org) + token_type = self.detect_token_type(token) if token else "unknown" + git_env = self._build_git_env(token) + + ctx = AuthContext( + token=token, + source=source, + token_type=token_type, + host_info=host_info, + git_env=git_env, + ) self._cache[key] = ctx - return ctx + return ctx def resolve_for_dep(self, dep_ref: "DependencyReference") -> AuthContext: """Resolve auth from a ``DependencyReference``.""" diff --git a/src/apm_cli/deps/github_downloader.py b/src/apm_cli/deps/github_downloader.py index 0d141d27d..37988ad5d 100644 --- a/src/apm_cli/deps/github_downloader.py +++ b/src/apm_cli/deps/github_downloader.py @@ -202,20 +202,19 @@ def _setup_git_environment(self) -> Dict[str, Any]: else: env['GIT_CONFIG_GLOBAL'] = '/dev/null' - # Resolve default host tokens via AuthResolver (backward compat properties) - default_ctx = self.auth_resolver.resolve(default_host()) - self._default_github_ctx = default_ctx - self.github_token = default_ctx.token - self.has_github_token = default_ctx.token is not None - self._github_token_from_credential_fill = ( - self.has_github_token - and self.token_manager.get_token_for_purpose('modules', env) is None - ) - - # Azure DevOps - ado_ctx = self.auth_resolver.resolve("dev.azure.com") - self.ado_token = ado_ctx.token - self.has_ado_token = ado_ctx.token is not None + # IMPORTANT: Do not resolve credentials via helpers at construction time. + # AuthResolver.resolve(...) can trigger OS credential helper UI. If we do + # this eagerly (host-only key) and later resolve per-dependency (host+org), + # users can see duplicate auth prompts. Keep constructor token state env-only + # and resolve lazily per dependency during clone/validate flows. + self._default_github_ctx = None + self.github_token = self.token_manager.get_token_for_purpose('modules', env) + self.has_github_token = self.github_token is not None + self._github_token_from_credential_fill = False + + # Azure DevOps (env-only at init; lazy auth resolution happens per dep) + self.ado_token = self.token_manager.get_token_for_purpose('ado_modules', env) + self.has_ado_token = self.ado_token is not None # JFrog Artifactory (not host-based, uses dedicated env var) self.artifactory_token = self.token_manager.get_token_for_purpose('artifactory_modules', env) diff --git a/tests/test_github_downloader.py b/tests/test_github_downloader.py index ec941ddf9..28c47d402 100644 --- a/tests/test_github_downloader.py +++ b/tests/test_github_downloader.py @@ -69,6 +69,15 @@ def test_setup_git_environment_no_token(self): # Should not have GitHub tokens in environment assert 'GITHUB_TOKEN' not in env or not env['GITHUB_TOKEN'] assert 'GH_TOKEN' not in env or not env['GH_TOKEN'] + + def test_setup_git_environment_does_not_eagerly_call_credential_helper(self): + """Constructor should not invoke git credential helper (lazy per-dep auth).""" + with patch.dict(os.environ, {}, clear=True): + with patch( + 'apm_cli.core.token_manager.GitHubTokenManager.resolve_credential_from_git' + ) as mock_cred: + GitHubPackageDownloader() + mock_cred.assert_not_called() @patch('apm_cli.deps.github_downloader.Repo') @patch('tempfile.mkdtemp') diff --git a/tests/unit/test_auth.py b/tests/unit/test_auth.py index 9daa10d09..ab63b3b2f 100644 --- a/tests/unit/test_auth.py +++ b/tests/unit/test_auth.py @@ -1,6 +1,8 @@ """Unit tests for AuthResolver, HostInfo, and AuthContext.""" import os +import time +from concurrent.futures import ThreadPoolExecutor from unittest.mock import patch import pytest @@ -134,6 +136,25 @@ def test_caching(self): ctx2 = resolver.resolve("github.com", org="microsoft") assert ctx1 is ctx2 + def test_caching_is_singleflight_under_concurrency(self): + """Concurrent resolve() calls for the same key should populate cache once.""" + resolver = AuthResolver() + + def _slow_resolve_token(host_info, org): + time.sleep(0.05) + return ("cred-token", "git-credential-fill") + + with patch.object(AuthResolver, "_resolve_token", side_effect=_slow_resolve_token) as mock_resolve: + with ThreadPoolExecutor(max_workers=8) as pool: + futures = [ + pool.submit(resolver.resolve, "github.com", "microsoft") + for _ in range(8) + ] + contexts = [f.result() for f in futures] + + assert mock_resolve.call_count == 1 + assert all(ctx is contexts[0] for ctx in contexts) + def test_different_orgs_different_cache(self): """Different orgs get different cache entries.""" with patch.dict(os.environ, { From 8ccff02ecad0da54b84a5fc6e66b1528e9738f0e Mon Sep 17 00:00:00 2001 From: Thomas Caudal Date: Mon, 23 Mar 2026 18:05:10 +0100 Subject: [PATCH 02/10] Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- dist.old/apm-windows-x86_64.sha256 | 1 - 1 file changed, 1 deletion(-) diff --git a/dist.old/apm-windows-x86_64.sha256 b/dist.old/apm-windows-x86_64.sha256 index bba469bf6..e69de29bb 100644 --- a/dist.old/apm-windows-x86_64.sha256 +++ b/dist.old/apm-windows-x86_64.sha256 @@ -1 +0,0 @@ -56b2be37fde6ccdef0074d696463980768078cf5ff39757e2837a7e05c6ff581 dist/apm-windows-x86_64/apm.exe From 45b151703cbf0ae653b38a1537d77f4ea7e25afa Mon Sep 17 00:00:00 2001 From: "ENABLON\\thomas.caudal" Date: Mon, 23 Mar 2026 18:12:54 +0100 Subject: [PATCH 03/10] fix: move AuthResolver import to improve clarity and prevent potential issues --- src/apm_cli/commands/install.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index 7d1c39a1f..7bdaf5804 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -35,11 +35,12 @@ list = builtins.list dict = builtins.dict +from ..core.auth import AuthResolver + # APM Dependencies (conditional import for graceful degradation) APM_DEPS_AVAILABLE = False _APM_IMPORT_ERROR = None try: - from ..core.auth import AuthResolver from ..deps.apm_resolver import APMDependencyResolver from ..deps.github_downloader import GitHubPackageDownloader from ..deps.lockfile import LockFile, get_lockfile_path, migrate_lockfile_if_needed From 3867ea3547e021930cd188e661448c0f1d651f16 Mon Sep 17 00:00:00 2001 From: "ENABLON\\thomas.caudal" Date: Wed, 25 Mar 2026 09:00:41 -0400 Subject: [PATCH 04/10] fix: remove accidentally tracked dist.old build artifact The dist.old/apm-windows-x86_64.sha256 checksum file was an accidental build artifact committed to the repo. Nothing in the codebase references dist.old/, and dist/ is already in .gitignore. --- dist.old/apm-windows-x86_64.sha256 | 0 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 dist.old/apm-windows-x86_64.sha256 diff --git a/dist.old/apm-windows-x86_64.sha256 b/dist.old/apm-windows-x86_64.sha256 deleted file mode 100644 index e69de29bb..000000000 From 30f7b3bd8897cd63d7793d8d5136b6eb2c07aa08 Mon Sep 17 00:00:00 2001 From: "ENABLON\\thomas.caudal" Date: Wed, 25 Mar 2026 09:13:58 -0400 Subject: [PATCH 05/10] fix: reuse shared auth_resolver for verbose auth logging in install The verbose auth logging block inside _install_apm_dependencies was creating a fresh AuthResolver() instance instead of reusing the shared resolver passed to the function. This meant the resolved credentials were not cached, potentially triggering duplicate credential-helper popups when the resolver was used for logging after downloads. Replace the per-verbose-block AuthResolver() construction with direct use of the auth_resolver parameter already available in scope. --- src/apm_cli/commands/install.py | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index 7bdaf5804..bfcdaa4bb 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -1440,13 +1440,9 @@ def _collect_descendants(node, visited=None): _pre_downloaded_keys = builtins.set(_pre_download_results.keys()) # Create progress display for sequential integration - _auth_resolver = None - if verbose: - try: - from apm_cli.core.auth import AuthResolver - _auth_resolver = AuthResolver() - except Exception: - pass + # Reuse the shared auth_resolver (already created in this invocation) so + # verbose auth logging does not trigger a duplicate credential-helper popup. + _auth_resolver = auth_resolver with Progress( SpinnerColumn(), From 15b49cf0948e74088213a5fc25a82e52cb1eaeb3 Mon Sep 17 00:00:00 2001 From: "ENABLON\\thomas.caudal" Date: Wed, 25 Mar 2026 09:18:44 -0400 Subject: [PATCH 06/10] fix: resolve per-dep auth before building GHES/ADO validation URL In _validate_package_exists, the URL for git ls-remote was built via _build_repo_url() without a token for GHES and ADO hosts. Since the downloader constructor no longer resolves credentials eagerly (to prevent duplicate auth popups), self.github_token / self.ado_token are now env-only. Credential-helper-only users on GHES/ADO would therefore receive a tokenless ls-remote URL and see an OS credential prompt -- reintroducing exactly the problem this PR targets. Fix: call auth_resolver.resolve_for_dep(dep_ref) for non-generic (GHES/ADO) hosts before building the URL, and pass the resolved token to _build_repo_url(token=...). Generic hosts are excluded because they rely on git credential helpers through the relaxed validate_env, not on APM-managed auth. --- src/apm_cli/commands/install.py | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index bfcdaa4bb..aa1137070 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -273,20 +273,33 @@ def _validate_package_exists(package, verbose=False, auth_resolver=None): if dep_ref.is_azure_devops() or (dep_ref.host and dep_ref.host != "github.com"): from apm_cli.utils.github_host import is_github_hostname, is_azure_devops_hostname + # Determine host type before building the URL so we know whether to + # embed a token. Generic (non-GitHub, non-ADO) hosts are excluded + # from APM-managed auth; they rely on git credential helpers via the + # relaxed validate_env below. + is_generic = not is_github_hostname(dep_ref.host) and not is_azure_devops_hostname(dep_ref.host) + + # For GHES / ADO: resolve per-dependency auth up front so the URL + # carries an embedded token and avoids triggering OS credential + # helper popups during git ls-remote validation. + _url_token = None + if not is_generic: + _dep_ctx = auth_resolver.resolve_for_dep(dep_ref) + _url_token = _dep_ctx.token + ado_downloader = GitHubPackageDownloader(auth_resolver=auth_resolver) # Set the host if dep_ref.host: ado_downloader.github_host = dep_ref.host - # Build authenticated URL using downloader's auth + # Build authenticated URL using the resolved per-dep token. package_url = ado_downloader._build_repo_url( - dep_ref.repo_url, use_ssh=False, dep_ref=dep_ref + dep_ref.repo_url, use_ssh=False, dep_ref=dep_ref, token=_url_token ) # For generic hosts (not GitHub, not ADO), relax the env so native # credential helpers (SSH keys, macOS Keychain, etc.) can work. # This mirrors _clone_with_fallback() which does the same relaxation. - is_generic = not is_github_hostname(dep_ref.host) and not is_azure_devops_hostname(dep_ref.host) if is_generic: validate_env = {k: v for k, v in ado_downloader.git_env.items() if k not in ('GIT_ASKPASS', 'GIT_CONFIG_GLOBAL', 'GIT_CONFIG_NOSYSTEM')} From 33423a653384c48280a973e5991eb045f1c2ddee Mon Sep 17 00:00:00 2001 From: "ENABLON\\thomas.caudal" Date: Wed, 25 Mar 2026 09:23:00 -0400 Subject: [PATCH 07/10] fix: use resolved dep_token in clone error handler for credential-helper users The error handler in _clone_with_fallback used self.has_github_token to determine whether to show an 'authentication not configured' message. Since the constructor no longer calls auth_resolver.resolve() eagerly, self.has_github_token reflects env-var tokens only. Credential-helper- only users (gh auth login, macOS Keychain, etc.) have has_github_token=False even when credentials are available via helper, causing a misleading 'set up authentication' error when the actual cause is a permissions or repo-not-found issue. Switch to has_token, which is derived from dep_ctx.token (the result of per-dependency resolve_for_dep(), including credential-helper tokens). When has_token is True but clone still fails, the error correctly falls through to the generic 'check permissions' message. When has_token is False (truly no auth configured), the auth setup guidance is shown. --- src/apm_cli/deps/github_downloader.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/apm_cli/deps/github_downloader.py b/src/apm_cli/deps/github_downloader.py index 37988ad5d..206e1d2cc 100644 --- a/src/apm_cli/deps/github_downloader.py +++ b/src/apm_cli/deps/github_downloader.py @@ -663,7 +663,9 @@ def _clone_with_fallback(self, repo_url_base: str, target_path: Path, progress_r f"If this package lives on a different server (e.g., github.com), " f"use the full hostname in apm.yml: {suggested}" ) - elif not self.has_github_token: + elif not has_token: + # No auth was resolved (neither env var nor credential helper). + # Guide the user through setting up authentication. host = dep_host or default_host() org = dep_ref.repo_url.split('/')[0] if dep_ref and dep_ref.repo_url else None error_msg += self.auth_resolver.build_error_context(host, "clone", org=org) From 7552f76f7f8243960af658d0409826db70d2cd57 Mon Sep 17 00:00:00 2001 From: "ENABLON\\thomas.caudal" Date: Wed, 25 Mar 2026 09:23:35 -0400 Subject: [PATCH 08/10] docs: clarify why AuthResolver is imported outside APM_DEPS_AVAILABLE guard AuthResolver has no optional dependencies (stdlib + internal utils only). Add comments explaining it must be imported unconditionally, not inside the APM_DEPS_AVAILABLE try/except block: if an optional dep like GitPython is missing, a gated import would produce a NameError inside install() before the graceful APM_DEPS_AVAILABLE guard can produce a useful error. --- src/apm_cli/commands/install.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index aa1137070..82eb60233 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -35,6 +35,10 @@ list = builtins.list dict = builtins.dict +# AuthResolver has no optional deps (stdlib + internal utils only), so it must +# be imported unconditionally here -- NOT inside the APM_DEPS_AVAILABLE guard. +# If it were gated, a missing optional dep (e.g. GitPython) would cause a +# NameError in install() before the graceful APM_DEPS_AVAILABLE check fires. from ..core.auth import AuthResolver # APM Dependencies (conditional import for graceful degradation) From 10f86242f21de6077e5b3ba9dc43040c7988404e Mon Sep 17 00:00:00 2001 From: "ENABLON\\thomas.caudal" Date: Wed, 25 Mar 2026 09:24:05 -0400 Subject: [PATCH 09/10] docs: document lock hold trade-off in AuthResolver.resolve() Add a comment explaining why the lock is held for the full duration of credential resolution, not just for the cache check/write. Key points: - Prevents duplicate OS credential-helper popups under parallel downloads - First caller fills cache; subsequent same-key calls are O(1) hits - Bounded by APM_GIT_CREDENTIAL_TIMEOUT (default 60s) - No deadlock risk: single lock, never nested --- src/apm_cli/core/auth.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/apm_cli/core/auth.py b/src/apm_cli/core/auth.py index dc569e5bf..fc18d4ca8 100644 --- a/src/apm_cli/core/auth.py +++ b/src/apm_cli/core/auth.py @@ -186,8 +186,12 @@ def resolve(self, host: str, org: Optional[str] = None) -> AuthContext: if cached is not None: return cached - # Keep cache fill inside the lock to avoid concurrent duplicate - # credential-helper lookups for the same host/org. + # Hold lock during entire credential resolution to prevent duplicate + # credential-helper popups when parallel downloads resolve the same + # (host, org) concurrently. The first caller fills the cache; all + # subsequent callers for the same key become O(1) cache hits. + # Bounded by APM_GIT_CREDENTIAL_TIMEOUT (default 60s). No deadlock + # risk: single lock, never nested. host_info = self.classify_host(host) token, source = self._resolve_token(host_info, org) token_type = self.detect_token_type(token) if token else "unknown" From e7e44afb5071df874ee90f27f2e59a5fef578df0 Mon Sep 17 00:00:00 2001 From: "ENABLON\\thomas.caudal" Date: Wed, 25 Mar 2026 09:30:32 -0400 Subject: [PATCH 10/10] refactor: remove unused _default_github_ctx field from GitHubPackageDownloader The field was assigned in _setup_git_environment() but never read anywhere in the codebase. Remove it to eliminate dead code and reduce confusion about its purpose. --- src/apm_cli/deps/github_downloader.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/apm_cli/deps/github_downloader.py b/src/apm_cli/deps/github_downloader.py index 206e1d2cc..a54800eba 100644 --- a/src/apm_cli/deps/github_downloader.py +++ b/src/apm_cli/deps/github_downloader.py @@ -207,7 +207,6 @@ def _setup_git_environment(self) -> Dict[str, Any]: # this eagerly (host-only key) and later resolve per-dependency (host+org), # users can see duplicate auth prompts. Keep constructor token state env-only # and resolve lazily per dependency during clone/validate flows. - self._default_github_ctx = None self.github_token = self.token_manager.get_token_for_purpose('modules', env) self.has_github_token = self.github_token is not None self._github_token_from_credential_fill = False