DX: Improve auth error messages, sparse checkout token consistency, and error deduplication

CHANGELOG.md

@@ -10,6 +10,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- Misleading "transitive dep" error message for direct dependency download failures (#478)
+- Sparse checkout using global token instead of per-org token from `GITHUB_APM_PAT_<ORG>` (#478)
+- Duplicate error count when a dependency fails during both resolution and install phases (#478)
 - Windows Defender false-positive (`Trojan:Win32/Bearfoos.B!ml`) mitigation: embed PE version info in Windows binary and disable UPX compression on Windows builds (#487)
 - `apm deps update` was a no-op -- rewrote to delegate to the install engine so lockfile, deployed files, and integration state are all refreshed correctly -- by @webmaxru (#493)
 

src/apm_cli/commands/install.py

@@ -1174,7 +1174,7 @@ def _install_apm_dependencies(
     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)
+    direct_dep_keys = builtins.set(dep.get_unique_key() for dep in all_apm_deps)
 
     # Track paths already downloaded by the resolver callback to avoid re-downloading
     # Maps dep_key -> resolved_commit (SHA or None) so the cached path can use it
@@ -1184,6 +1184,10 @@ def _install_apm_dependencies(
     # diagnostics after the DiagnosticCollector is created (later in the flow).
     transitive_failures: list[tuple[str, str]] = []  # (dep_display, message)
 
+    # Track dep keys that failed in download_callback so the main install loop
+    # skips them instead of re-trying and producing a duplicate error entry.
+    callback_failures: builtins.set = builtins.set()
+
     # Create a download callback for transitive dependency resolution
     # This allows the resolver to fetch packages on-demand during tree building
     def download_callback(dep_ref, modules_dir, parent_chain=""):
@@ -1231,19 +1235,31 @@ def download_callback(dep_ref, modules_dir, parent_chain=""):
             callback_downloaded[dep_ref.get_unique_key()] = resolved_sha
             return install_path
         except Exception as e:
-            # Build contextual message including the dependency chain breadcrumb
-            chain_hint = f" (via {parent_chain})" if parent_chain else ""
             dep_display = dep_ref.get_display_name()
-            fail_msg = (
-                f"Failed to resolve transitive dep "
-                f"{dep_ref.repo_url}{chain_hint}: {e}"
-            )
+            dep_key = dep_ref.get_unique_key()
+            is_direct = dep_key in direct_dep_keys
+
+            # Distinguish direct vs transitive failure messages so users
+            # don't see a misleading "transitive dep" label for top-level deps.
+            if is_direct:
+                fail_msg = (
+                    f"Failed to download dependency "
+                    f"{dep_ref.repo_url}: {e}"
+                )
+            else:
+                chain_hint = f" (via {parent_chain})" if parent_chain else ""
+                fail_msg = (
+                    f"Failed to resolve transitive dep "
+                    f"{dep_ref.repo_url}{chain_hint}: {e}"
+                )
+
             # Verbose: inline detail
             if logger:
                 logger.verbose_detail(f"  {fail_msg}")
             elif verbose:
                 _rich_error(f"  └─ {fail_msg}")
             # Collect for deferred diagnostics summary (always, even non-verbose)
+            callback_failures.add(dep_key)
             transitive_failures.append((dep_display, fail_msg))
             return None
 
@@ -1556,6 +1572,14 @@ def _collect_descendants(node, visited=None):
                     # Use the canonical install path from DependencyReference
                     install_path = dep_ref.get_install_path(apm_modules_dir)
 
+                # Skip deps that already failed during BFS resolution callback
+                # to avoid a duplicate error entry in diagnostics.
+                dep_key = dep_ref.get_unique_key()
+                if dep_key in callback_failures:
+                    if logger:
+                        logger.verbose_detail(f"  Skipping {dep_key} (already failed during resolution)")
+                    continue
+
                 # --- Local package: copy from filesystem (no git download) ---
                 if dep_ref.is_local and dep_ref.local_path:
                     result_path = _copy_local_package(dep_ref, install_path, project_root)

src/apm_cli/deps/github_downloader.py

@@ -358,6 +358,36 @@ def _parse_artifactory_base_url(self) -> Optional[tuple]:
             return None
         return (host, path, parsed.scheme)
 
+    def _resolve_dep_token(self, dep_ref: Optional[DependencyReference] = None) -> Optional[str]:
+        """Resolve the per-dependency auth token via AuthResolver.
+
+        GitHub and ADO hosts use the token resolved by AuthResolver.
+        Generic hosts (GitLab, Bitbucket, etc.) return None so git
+        credential helpers can provide credentials instead.
+
+        Args:
+            dep_ref: Optional dependency reference for host/org lookup.
+
+        Returns:
+            Token string or None.
+        """
+        if dep_ref is None:
+            return self.github_token
+
+        is_ado = dep_ref.is_azure_devops()
+        dep_host = dep_ref.host
+        if dep_host:
+            is_github = is_github_hostname(dep_host)
+        else:
+            is_github = True
+        is_generic = not is_ado and not is_github
+
+        if is_generic:
+            return None
+
+        dep_ctx = self.auth_resolver.resolve_for_dep(dep_ref)
+        return dep_ctx.token
+
     def _resilient_get(self, url: str, headers: Dict[str, str], timeout: int = 30, max_retries: int = 3) -> requests.Response:
         """HTTP GET with retry on 429/503 and rate-limit header awareness (#171).
 
@@ -566,15 +596,7 @@ def _clone_with_fallback(self, repo_url_base: str, target_path: Path, progress_r
         is_generic = not is_ado and not is_github
 
         # Resolve per-dependency token via AuthResolver.
-        # Only use resolved token for GitHub/ADO hosts — generic hosts (GitLab,
-        # Bitbucket, etc.) delegate auth to git credential helpers.
-        if dep_ref and not is_generic:
-            dep_ctx = self.auth_resolver.resolve_for_dep(dep_ref)
-            dep_token = dep_ctx.token
-        elif is_generic:
-            dep_token = None
-        else:
-            dep_token = self.github_token  # fallback
+        dep_token = self._resolve_dep_token(dep_ref)
         has_token = dep_token
 
         _debug(f"_clone_with_fallback: repo={repo_url_base}, is_ado={is_ado}, is_generic={is_generic}, has_token={has_token is not None}")
@@ -1459,8 +1481,12 @@ def _try_sparse_checkout(self, dep_ref: DependencyReference, temp_clone_path: Pa
         import subprocess
         try:
             temp_clone_path.mkdir(parents=True, exist_ok=True)
+
+            # Resolve per-dependency token via AuthResolver.
+            dep_token = self._resolve_dep_token(dep_ref)
+
             env = {**os.environ, **(self.git_env or {})}
-            auth_url = self._build_repo_url(dep_ref.repo_url, use_ssh=False, dep_ref=dep_ref)
+            auth_url = self._build_repo_url(dep_ref.repo_url, use_ssh=False, dep_ref=dep_ref, token=dep_token)
 
             cmds = [
                 ['git', 'init'],

tests/unit/test_auth_scoping.py

@@ -596,3 +596,48 @@ def test_ghe_host_is_github(self):
         dep_ref = _dep("https://company.ghe.com/org/repo.git")
         from apm_cli.utils.github_host import is_github_hostname
         assert is_github_hostname(dep_ref.host) is True
+
+
+# ===========================================================================
+# _try_sparse_checkout -- per-dep token resolution
+# ===========================================================================
+
+class TestSparseCheckoutTokenResolution:
+    """Verify _try_sparse_checkout uses resolve_for_dep() for per-dep tokens."""
+
+    def test_sparse_checkout_uses_per_org_token(self, tmp_path):
+        """Sparse checkout should use per-org token, not the global instance token."""
+        org_token = "ghp_ORG_SPECIFIC"
+        global_token = "ghp_GLOBAL"
+
+        with patch.dict(os.environ, {
+            "GITHUB_APM_PAT": global_token,
+            "GITHUB_APM_PAT_ACME": org_token,
+        }, clear=True), patch(
+            "apm_cli.core.token_manager.GitHubTokenManager.resolve_credential_from_git",
+            return_value=None,
+        ):
+            dl = GitHubPackageDownloader()
+            dep = _dep("acme/mono-repo/subdir")
+
+            # Patch subprocess.run to capture the URL used in 'git remote add'
+            captured_urls = []
+
+            def capture_run(cmd, **kwargs):
+                if len(cmd) >= 5 and cmd[:3] == ["git", "remote", "add"]:
+                    captured_urls.append(cmd[4])  # The URL argument (after 'origin')
+                    # Fail after capturing to keep the test fast
+                    return MagicMock(returncode=1, stderr="test abort")
+                # Let other commands (git init, etc.) succeed
+                return MagicMock(returncode=0, stderr="")
+
+            with patch("subprocess.run", side_effect=capture_run):
+                dl._try_sparse_checkout(dep, tmp_path / "sparse", "subdir", ref="main")
+
+            assert len(captured_urls) == 1, f"Expected 1 URL capture, got {captured_urls}"
+            # The per-org token should be in the URL, not the global one
+            assert org_token in captured_urls[0], (
+                f"Expected org-specific token in sparse checkout URL, "
+                f"got: {captured_urls[0]}"
+            )
+            assert global_token not in captured_urls[0]

tests/unit/test_install_command.py

@@ -491,6 +491,91 @@ def tracking_callback(dep_ref, mods_dir, parent_chain=""):
         )
 
 
+class TestDownloadCallbackErrorMessages:
+    """Tests for direct vs transitive dep error message differentiation."""
+
+    def test_direct_dep_failure_says_download_dependency(self, tmp_path, monkeypatch):
+        """Direct dependency failure uses 'Failed to download dependency', not 'transitive dep'."""
+        from apm_cli.commands.install import _install_apm_dependencies
+        from apm_cli.models.apm_package import APMPackage
+
+        monkeypatch.chdir(tmp_path)
+
+        # Create a minimal apm.yml with a direct dep
+        (tmp_path / "apm.yml").write_text(yaml.safe_dump({
+            "name": "test-project",
+            "version": "0.0.1",
+            "dependencies": {"apm": ["acme/direct-pkg"], "mcp": []},
+        }))
+
+        apm_package = APMPackage.from_apm_yml(tmp_path / "apm.yml")
+
+        # Patch the downloader to always fail
+        with patch("apm_cli.commands.install.GitHubPackageDownloader") as MockDownloader:
+            mock_dl = MockDownloader.return_value
+            mock_dl.download_package.side_effect = RuntimeError("auth failed")
+
+            result = _install_apm_dependencies(
+                apm_package, verbose=False, force=False, parallel_downloads=0,
+            )
+
+        # Check that the error message says "download dependency", not "transitive dep"
+        errors = result.diagnostics.by_category().get("error", [])
+        assert len(errors) == 1, f"Expected 1 error, got {len(errors)}: {errors}"
+        assert "Failed to download dependency" in errors[0].message
+        assert "transitive" not in errors[0].message.lower()
+
+    def test_transitive_dep_key_not_in_direct_dep_keys(self):
+        """Transitive dep keys are correctly absent from direct_dep_keys set.
+
+        The download_callback uses this check to select the right error label.
+        End-to-end transitive error flow is covered by
+        TestTransitiveDepParentChain.test_download_callback_includes_chain_in_error.
+        """
+        from apm_cli.models.apm_package import DependencyReference
+
+        direct_dep_keys = {"acme/root-pkg"}
+        transitive_ref = DependencyReference.parse("other-org/leaf-pkg")
+
+        # Transitive deps must NOT be in the direct set
+        assert transitive_ref.get_unique_key() not in direct_dep_keys
+        # Direct deps must be in the direct set
+        assert "acme/root-pkg" in direct_dep_keys
+
+
+class TestCallbackFailureDeduplication:
+    """Tests for error deduplication when download_callback failures are not re-tried."""
+
+    def test_callback_failure_not_duplicated_in_main_loop(self, tmp_path, monkeypatch):
+        """A dep that fails in download_callback should produce only one error."""
+        from apm_cli.commands.install import _install_apm_dependencies
+        from apm_cli.models.apm_package import APMPackage
+
+        monkeypatch.chdir(tmp_path)
+
+        (tmp_path / "apm.yml").write_text(yaml.safe_dump({
+            "name": "test-project",
+            "version": "0.0.1",
+            "dependencies": {"apm": ["acme/failing-pkg"], "mcp": []},
+        }))
+        apm_package = APMPackage.from_apm_yml(tmp_path / "apm.yml")
+
+        with patch("apm_cli.commands.install.GitHubPackageDownloader") as MockDownloader:
+            mock_dl = MockDownloader.return_value
+            mock_dl.download_package.side_effect = RuntimeError("auth failed")
+
+            result = _install_apm_dependencies(
+                apm_package, verbose=False, force=False, parallel_downloads=0,
+            )
+
+        errors = result.diagnostics.by_category().get("error", [])
+        # Should be exactly 1 error, not 2 (one from callback + one from main loop)
+        assert len(errors) == 1, (
+            f"Expected 1 error (deduplicated), got {len(errors)}: "
+            f"{[e.message for e in errors]}"
+        )
+
+
 class TestLocalPathValidationMessages:
     """Tests for improved local path validation error messages."""