fix(ado): resolve az binary via shutil.which for Windows az.cmd (closes #1430) (#1432)

* fix(ado): resolve az binary via shutil.which for Windows az.cmd (closes #1430)

On Windows the Azure CLI ships as az.cmd (a batch wrapper). Python's
subprocess.run -> CreateProcessW does NOT honor PATHEXT for non-.exe
executables, so a bare "az" token cannot be resolved and raises
FileNotFoundError. shutil.which("az") DOES honor PATHEXT and finds
az.cmd, so AzureCliBearerProvider.is_available() returned True while
every actual subprocess call failed. The error then cascaded through
the ADO --update preflight, killing Git Credential Manager along the
way, and surfaced as the misleading 'az present but not logged in'
diagnostic -- even when the user WAS logged in via az login.

Layer 1 (root cause): resolve the az executable once in __init__ via
shutil.which and store the absolute path. Use it in every subprocess
call (_run_get_access_token, get_current_tenant_id). is_available()
now mirrors the same construction-time resolution so it can no longer
disagree with get_bearer_token()'s pre-check. Explicit absolute /
separator-bearing paths from callers (tests) are trusted verbatim.

Layer 2 (defense in depth): pipeline.py _preflight_auth_check now
strips GIT_CONFIG_GLOBAL / GIT_CONFIG_NOSYSTEM / GIT_ASKPASS from the
probe env for ADO hosts too (previously only generic). This lets Git
Credential Manager answer for Entra-cached ADO credentials when az
bearer acquisition is unavailable for any reason -- the #1430 PATH
quirk, sandbox, proxy, etc. The actual clone path remains isolated
(it builds its own clean env via GitAuthEnvBuilder.setup_environment),
so the security rationale for empty global gitconfig during clone is
preserved. Only the single ls-remote probe leg gains GCM access.

Tests:
- New TestWindowsAzCmdResolution class with the exact regression
  shape: shutil.which returns a Windows az.cmd path; verify subprocess
  receives it as argv[0] for both get_bearer_token and tenant probe.
- New is_available stability test (resolution is one-shot).
- Init absolute-path bypass test (preserves test injection contract).
- Existing test_ado_host_retains_credential_blocking_env renamed and
  inverted to assert the new contract: ADO preflight no longer kills
  GCM. Existing ADO bearer-fallback E2E unchanged (the fake-git path
  has no GCM installed so behavior is unaffected).

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

* fix(ado): address panel review (#1430 round 2)

Three nits surfaced by auth-expert + supply-chain-security panel:

1. Tighten the explicit-path bypass in AzureCliBearerProvider.__init__
   from 'isabs OR contains os.sep' to 'isabs only'. The os.sep heuristic
   accepted relative-with-separator tokens like 'subdir/az' verbatim,
   handing a CWD-relative path to subprocess.run without going through
   shutil.which. Only absolute paths are now trusted verbatim; everything
   else flows through resolution. New test
   test_init_relative_with_separator_still_resolves locks the contract.

2. Add an explicit early-return in get_current_tenant_id when
   _az_command is None. Previously this case relied on the broad
   'except Exception' to swallow a TypeError from passing None as
   argv[0]. The new guard mirrors get_bearer_token's is_available()
   pre-check. New test
   test_get_current_tenant_id_returns_none_without_subprocess_when_az_missing
   asserts subprocess.run is never invoked in that path.

3. Add an explicit FileNotFoundError regression-trap test
   (test_bare_az_would_raise_filenotfound_but_resolved_path_succeeds)
   that simulates the exact Windows CreateProcessW behaviour: argv[0]
   == 'az' raises FileNotFoundError, argv[0] == resolved .cmd path
   succeeds. Pins both halves of the #1430 contract in a single test.

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

* docs(changelog): add #1430 entry under Unreleased > Fixed

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

---------

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

Author: danielmeppiel

CHANGELOG.md

@@ -13,6 +13,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- `apm update` against private Azure DevOps deps no longer fails on Windows with a misleading "az present but not logged in" diagnostic when the user IS signed in via `az login`. Root cause: Python's `subprocess.run(["az", ...])` -> `CreateProcessW` does not honor `PATHEXT` for non-`.exe` executables, so the Windows `az.cmd` wrapper could not be invoked even though `shutil.which("az")` resolved it. `AzureCliBearerProvider` now resolves the `az` binary via `shutil.which` once at construction and passes the absolute path to every subprocess call. As a defense-in-depth measure, the ADO `--update` preflight probe no longer strips `GIT_CONFIG_GLOBAL` / `GIT_CONFIG_NOSYSTEM` / `GIT_ASKPASS`, so Git Credential Manager can answer for Entra-cached ADO credentials whenever bearer acquisition is unavailable for any reason (sandbox, proxy, future PATH quirks). The actual clone path keeps its full gitconfig isolation. (#1430)
+
 - Root `.apm` hooks no longer duplicate after renaming the project directory or using git worktrees; Claude, Codex, Cursor, Gemini, and Windsurf hook configs stay idempotent across checkouts. The hook source-id is now derived from `apm.yml`'s `name` field instead of `install_path.name`, and `apm install` silently heals stale same-content entries from prior checkout basenames. Copilot is unaffected (its hooks live in per-file namespaces under `.github/hooks/`, not a shared merged config). (#1392, closes #1329)
 
 ## [0.14.1] - 2026-05-20

src/apm_cli/core/azure_cli.py

@@ -20,6 +20,7 @@
 from __future__ import annotations
 
 import json
+import os
 import shutil
 import subprocess
 import threading
@@ -82,7 +83,25 @@ class AzureCliBearerProvider:
     _EXPIRY_SLACK_SECONDS: int = 60
 
     def __init__(self, az_command: str = "az") -> None:
-        self._az_command = az_command
+        # Resolve the az executable once at construction. On Windows the
+        # Azure CLI ships as az.cmd (a batch wrapper), and Python's
+        # subprocess.run -> CreateProcessW does NOT honor PATHEXT for
+        # non-.exe executables, so a bare "az" token fails with
+        # FileNotFoundError even though `shutil.which("az")` resolves it.
+        # See microsoft/apm#1430. Resolving via shutil.which here (which
+        # DOES honor PATHEXT) and storing the absolute path keeps every
+        # downstream subprocess.run call portable across platforms.
+        #
+        # Edge case: callers (notably tests) may pass an explicit absolute
+        # path. We trust absolute paths verbatim and skip re-resolution.
+        # Relative tokens always flow through shutil.which so a
+        # CWD-relative form like "subdir/az" cannot bypass resolution.
+        #
+        # PATH changes mid-process won't be detected; restart the CLI.
+        if os.path.isabs(az_command):
+            self._az_command: str | None = az_command
+        else:
+            self._az_command = shutil.which(az_command)
         # Cache stores (token, expires_at_epoch_seconds). expires_at is None
         # if the response did not include an expiresOn field (very old az
         # versions); in that case the token is treated as never-expiring
@@ -93,12 +112,16 @@ def __init__(self, az_command: str = "az") -> None:
     # -- public API ---------------------------------------------------------
 
     def is_available(self) -> bool:
-        """Return True iff the ``az`` binary is on PATH.
+        """Return True iff the ``az`` binary was found on PATH at construction.
+
+        Resolution is one-shot (performed in :meth:`__init__`). PATH changes
+        made after the provider was constructed will NOT be observed; restart
+        the CLI if you have installed ``az`` mid-session.
 
         Does NOT check whether the user is logged in -- that requires a
         subprocess call and is deferred to :meth:`get_bearer_token`.
         """
-        return shutil.which(self._az_command) is not None
+        return self._az_command is not None
 
     def get_bearer_token(self) -> str:
         """Acquire (or return cached) bearer token for Azure DevOps.
@@ -146,6 +169,12 @@ def get_current_tenant_id(self) -> str | None:
         Uses ``az account show --query tenantId -o tsv``.  Returns ``None``
         on any failure -- this method never raises.
         """
+        # Explicit early-return when az was not resolved at __init__: avoids
+        # passing None as argv[0] to subprocess.run (which would TypeError
+        # and only be swallowed by the broad except below). Mirrors the
+        # is_available() guard that get_bearer_token() does.
+        if not self._az_command:
+            return None
         try:
             result = subprocess.run(
                 [self._az_command, "account", "show", "--query", "tenantId", "-o", "tsv"],

src/apm_cli/install/pipeline.py

@@ -157,7 +157,21 @@ def _trace(line: str) -> None:
         )
         _ctx_env = getattr(dep_ctx, "git_env", {}) or {}
         probe_env = {**os.environ, **_dl.git_env, **_ctx_env}
-        if is_generic:
+        # GIT_CONFIG_GLOBAL / GIT_CONFIG_NOSYSTEM carve-out: GitAuthEnvBuilder
+        # forces an empty global gitconfig for ALL hosts to prevent a user's
+        # ~/.gitconfig insteadOf rewrites or credential helpers from leaking
+        # tokens during a clone. But for preflight probes (a single ls-remote
+        # against the same host the dep targets), the redirection surface is
+        # nil and killing the user's global config kills Git Credential
+        # Manager along with it -- the helper most Windows ADO users rely on
+        # for Entra-cached credentials. For ADO specifically that matters
+        # because bearer acquisition can fail for reasons unrelated to login
+        # state (sandbox, proxy, microsoft/apm#1430-style PATH quirks), and
+        # GCM is the only remaining channel that can save us. Generic hosts
+        # have the same logic; widening the carve-out to ADO keeps the
+        # actual clone path isolated (it builds its own clean env) while
+        # giving the preflight probe the best chance to succeed.
+        if is_generic or is_azure_devops_hostname(host):
             for _key in ("GIT_CONFIG_GLOBAL", "GIT_CONFIG_NOSYSTEM", "GIT_ASKPASS"):
                 probe_env.pop(_key, None)
 

tests/unit/install/test_pipeline_auth_preflight.py

@@ -194,11 +194,17 @@ def test_generic_host_auth_failure_still_raises(self, mock_run):
         assert str(exc_info.value).startswith("Authentication failed for ghes.corp.example.com")
 
     @patch("subprocess.run")
-    def test_ado_host_retains_credential_blocking_env(self, mock_run):
-        """ADO hosts should retain GIT_ASKPASS (locked-down env with token in URL).
-
-        Generic hosts strip GIT_ASKPASS to allow credential helpers; ADO hosts
-        keep it because auth is via token embedded in the URL.
+    def test_ado_host_omits_credential_blocking_env(self, mock_run):
+        """ADO hosts strip GIT_CONFIG_GLOBAL / GIT_CONFIG_NOSYSTEM / GIT_ASKPASS
+        from the preflight probe env so Git Credential Manager can answer
+        for Entra-cached ADO creds when bearer acquisition is unavailable
+        (microsoft/apm#1430 -- Windows az.cmd resolution failure, sandboxed
+        runs, proxy issues, etc.).
+
+        The actual clone path remains isolated -- it builds its own clean
+        env via GitAuthEnvBuilder.setup_environment(). This carve-out only
+        widens the preflight PROBE leg so GCM gets a chance to authenticate
+        before we surface a misleading "az not logged in" diagnostic.
         """
         mock_run.return_value = MagicMock(returncode=0, stderr="", stdout="")
 
@@ -219,9 +225,10 @@ def test_ado_host_retains_credential_blocking_env(self, mock_run):
         _preflight_auth_check(ctx, resolver, verbose=False)
 
         call_env = mock_run.call_args[1]["env"]
-        # ADO hosts keep the locked-down env since tokens are embedded in the URL
-        assert call_env.get("GIT_CONFIG_NOSYSTEM") == "1"
-        assert call_env.get("GIT_ASKPASS") == "echo"
+        # Layer 2 of #1430 fix: ADO preflight no longer kills GCM.
+        assert "GIT_CONFIG_NOSYSTEM" not in call_env
+        assert "GIT_ASKPASS" not in call_env
+        assert "GIT_CONFIG_GLOBAL" not in call_env
 
 
 # ---------------------------------------------------------------------------

tests/unit/test_azure_cli.py

@@ -32,6 +32,152 @@ def test_is_available_when_az_missing(self):
             provider = AzureCliBearerProvider()
             assert provider.is_available() is False
 
+    def test_is_available_stable_after_construction(self):
+        """is_available reflects construction-time resolution only.
+
+        Once the provider is built, mid-process PATH changes (or shutil.which
+        side effects) must NOT flip the answer. This is the contract that
+        keeps is_available() consistent with get_bearer_token()'s pre-check
+        and avoids a "True now / az_not_found a moment later" race.
+        """
+        with patch("apm_cli.core.azure_cli.shutil.which", return_value="/usr/bin/az"):
+            provider = AzureCliBearerProvider()
+        # Re-enter a context where which() would now say "missing": the
+        # already-constructed provider must still report True.
+        with patch("apm_cli.core.azure_cli.shutil.which", return_value=None):
+            assert provider.is_available() is True
+
+
+# ---------------------------------------------------------------------------
+# Windows az.cmd resolution (regression for microsoft/apm#1430)
+# ---------------------------------------------------------------------------
+
+
+class TestWindowsAzCmdResolution:
+    """Regression: subprocess.run must receive the shutil.which-resolved
+    path, not the bare "az" token. On Windows the resolver returns
+    "C:\\\\...\\\\az.cmd" and CreateProcessW cannot find "az" alone."""
+
+    WINDOWS_AZ_CMD = r"C:\Program Files (x86)\Microsoft SDKs\Azure\CLI2\wbin\az.cmd"
+
+    def test_init_resolves_via_shutil_which(self):
+        with patch(
+            "apm_cli.core.azure_cli.shutil.which", return_value=self.WINDOWS_AZ_CMD
+        ) as mock_which:
+            provider = AzureCliBearerProvider()
+            mock_which.assert_called_once_with("az")
+            assert provider._az_command == self.WINDOWS_AZ_CMD
+
+    def test_init_stores_none_when_not_on_path(self):
+        with patch("apm_cli.core.azure_cli.shutil.which", return_value=None):
+            provider = AzureCliBearerProvider()
+            assert provider._az_command is None
+
+    def test_init_absolute_path_skips_resolution(self):
+        """Caller passed an explicit absolute path -- trust verbatim."""
+        with patch("apm_cli.core.azure_cli.shutil.which") as mock_which:
+            provider = AzureCliBearerProvider(az_command="/opt/custom/az")
+            mock_which.assert_not_called()
+            assert provider._az_command == "/opt/custom/az"
+
+    def test_init_relative_with_separator_still_resolves(self):
+        """A relative-with-separator token like 'subdir/az' must NOT bypass
+        shutil.which; otherwise a caller could hand subprocess.run a
+        CWD-relative path that the OS resolves against the wrong
+        directory. Only absolute paths are trusted verbatim."""
+        with patch("apm_cli.core.azure_cli.shutil.which", return_value="/usr/bin/az") as mock_which:
+            provider = AzureCliBearerProvider(az_command="subdir/az")
+            mock_which.assert_called_once_with("subdir/az")
+            assert provider._az_command == "/usr/bin/az"
+
+    def test_get_bearer_token_invokes_resolved_az_cmd_path(self):
+        """The exact Windows shape: shutil.which returns az.cmd; verify it
+        flows into subprocess.run as the argv[0]. Without the fix, argv[0]
+        would be the bare "az" token and CreateProcessW would fail."""
+        mock_result = MagicMock()
+        mock_result.returncode = 0
+        mock_result.stdout = FAKE_JWT + "\n"
+        mock_result.stderr = ""
+
+        with (
+            patch("apm_cli.core.azure_cli.shutil.which", return_value=self.WINDOWS_AZ_CMD),
+            patch("apm_cli.core.azure_cli.subprocess.run", return_value=mock_result) as mock_run,
+        ):
+            provider = AzureCliBearerProvider()
+            provider.get_bearer_token()
+            assert mock_run.call_count == 1
+            cmd = mock_run.call_args[0][0]
+            assert cmd[0] == self.WINDOWS_AZ_CMD, (
+                f"subprocess.run must receive the resolved az.cmd path, got: {cmd[0]!r}"
+            )
+
+    def test_bare_az_would_raise_filenotfound_but_resolved_path_succeeds(self):
+        """Explicit regression trap for the #1430 cascade.
+
+        Pre-fix: subprocess.run(["az", ...]) raised FileNotFoundError on
+        Windows (CreateProcessW does not honor PATHEXT for az.cmd), which
+        _run_get_access_token caught as AzureCliBearerError(kind=
+        "subprocess_error"). The error propagated and was rendered as the
+        misleading "az present but not logged in" Case 3 diagnostic.
+
+        Post-fix: the constructor resolves to the .cmd absolute path, so
+        subprocess.run receives a path CreateProcessW CAN find and the
+        bearer succeeds. This test pins both halves of the contract:
+        bare 'az' -> FileNotFoundError; resolved path -> success.
+        """
+        mock_result = MagicMock()
+        mock_result.returncode = 0
+        mock_result.stdout = FAKE_JWT + "\n"
+        mock_result.stderr = ""
+
+        def fake_run(cmd, *args, **kwargs):
+            if cmd[0] == "az":
+                # Simulate the Windows CreateProcessW behavior pre-fix.
+                raise FileNotFoundError(2, "No such file or directory", "az")
+            return mock_result
+
+        with (
+            patch("apm_cli.core.azure_cli.shutil.which", return_value=self.WINDOWS_AZ_CMD),
+            patch("apm_cli.core.azure_cli.subprocess.run", side_effect=fake_run),
+        ):
+            provider = AzureCliBearerProvider()
+            # Would raise AzureCliBearerError(kind='subprocess_error') pre-fix.
+            token = provider.get_bearer_token()
+            assert token == FAKE_JWT
+
+    def test_get_current_tenant_id_invokes_resolved_az_cmd_path(self):
+        """Same regression for the get_current_tenant_id() probe -- this
+        was the second swallowed-failure that drove the misleading Case 3
+        diagnostic on Windows."""
+        mock_result = MagicMock()
+        mock_result.returncode = 0
+        mock_result.stdout = "72f988bf-86f1-41af-91ab-2d7cd011db47\n"
+        mock_result.stderr = ""
+
+        with (
+            patch("apm_cli.core.azure_cli.shutil.which", return_value=self.WINDOWS_AZ_CMD),
+            patch("apm_cli.core.azure_cli.subprocess.run", return_value=mock_result) as mock_run,
+        ):
+            provider = AzureCliBearerProvider()
+            provider.get_current_tenant_id()
+            assert mock_run.call_count == 1
+            cmd = mock_run.call_args[0][0]
+            assert cmd[0] == self.WINDOWS_AZ_CMD
+
+    def test_get_current_tenant_id_returns_none_without_subprocess_when_az_missing(self):
+        """Explicit early-return guard: when az was not resolved at __init__,
+        get_current_tenant_id() must short-circuit to None rather than
+        passing None as argv[0] (which would TypeError and only be caught
+        by the broad except). Mirrors get_bearer_token's is_available()
+        pre-check."""
+        with (
+            patch("apm_cli.core.azure_cli.shutil.which", return_value=None),
+            patch("apm_cli.core.azure_cli.subprocess.run") as mock_run,
+        ):
+            provider = AzureCliBearerProvider()
+            assert provider.get_current_tenant_id() is None
+            mock_run.assert_not_called()
+
 
 # ---------------------------------------------------------------------------
 # get_bearer_token