fix: share AuthResolver across install to prevent duplicate auth popups (#424)

* 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.

* Apply suggestion from @Copilot

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* fix: move AuthResolver import to improve clarity and prevent potential issues

* 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.

* 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.

* 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.

* 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.

* 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.

* 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

* 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.

---------

Co-authored-by: Daniel Meppiel <51440732+danielmeppiel@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Author: 3 people

src/apm_cli/commands/install.py

@@ -35,6 +35,12 @@
 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)
 APM_DEPS_AVAILABLE = False
 _APM_IMPORT_ERROR = None
@@ -56,7 +62,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 +74,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).
@@ -146,7 +153,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)
@@ -258,15 +265,17 @@ def _local_path_no_markers_hint(local_dir, verbose_log=None):
         _rich_echo(f"      ... and {len(found) - 5} more", color="dim")
 
 
-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
     import tempfile
     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
@@ -300,8 +309,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)
@@ -316,26 +325,39 @@ 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()
+            # 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:
-                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(
-                dep_ref.repo_url, use_ssh=False, dep_ref=dep_ref
+            # 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, 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 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}")
@@ -550,6 +572,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()
 
@@ -571,7 +597,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:
@@ -672,6 +698,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,
                     target=target,
                 )
                 apm_count = install_result.installed_count
@@ -1092,6 +1119,7 @@ def _install_apm_dependencies(
     force: bool = False,
     parallel_downloads: int = 4,
     logger: "InstallLogger" = None,
+    auth_resolver: "AuthResolver" = None,
     target: str = None,
 ):
     """Install APM package dependencies.
@@ -1104,6 +1132,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
         target: Explicit target override from --target CLI flag
     """
     if not APM_DEPS_AVAILABLE:
@@ -1137,8 +1166,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)
@@ -1499,13 +1532,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(),

src/apm_cli/core/auth.py

@@ -182,25 +182,30 @@ 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,
-        )
-
-        with self._lock:
+            cached = self._cache.get(key)
+            if cached is not None:
+                return cached
+
+            # 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"
+            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``."""

src/apm_cli/deps/github_downloader.py

@@ -186,20 +186,18 @@ 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.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)
@@ -648,7 +646,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)

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')

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, {