Address PR review comments for Python in-process transport

- Remove in-process PATH resolution of the CLI entrypoint (#1467): a bare
  `copilot` name resolved differently for provisioning vs loading, orphaning
  the native lib. In-process entrypoints are always absolute; no PATH lookup.
- Fix FFI-host cancellation ownership (#3972): assign self._ffi_host/_process
  before the blocking handshake so a cancelled start remains disposable.
- Honor an empty per-connection env dict in child-process CLI-path resolution
  (#1422): use explicit `is not None` checks instead of truthiness.
- Fail closed on missing/unsupported runtime-lib integrity (#418): refuse to
  load unverified native code; raise on unsupported hash algorithms. Add a
  regression test (test_cli_download.py).
- Document the empty-except temp-file cleanup (CodeQL #433).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: a96d1a82-c80e-40f1-a3df-a9708429b68b

Author: SteveSandersonMS

python/copilot/_cli_download.py

@@ -345,7 +345,12 @@ def _verify_integrity(data: bytes, integrity: str) -> None:
     algo, _, b64 = integrity.partition("-")
     algo = algo.lower()
     if algo not in ("sha512", "sha384", "sha256"):
-        return  # Unknown/unsupported algorithm — skip rather than fail.
+        # Fail closed: an unrecognized algorithm means we cannot verify this native
+        # library, so refuse rather than loading unverified native code.
+        raise RuntimeError(
+            f"Unsupported integrity algorithm '{algo}' for the in-process runtime "
+            "library; refusing to load unverified native code."
+        )
     expected = base64.b64decode(b64)
     actual = hashlib.new(algo, data).digest()
     if actual != expected:
@@ -414,8 +419,17 @@ def ensure_runtime_library(cli_path: str, version: str | None = None) -> str | N
     data = _fetch_url_bytes(url, timeout=600)
 
     integrity = _fetch_runtime_integrity(npm_platform, ver)
-    if integrity:
-        _verify_integrity(data, integrity)
+    if not integrity:
+        # Fail closed: this native library is loaded into the host process, so it must
+        # be verified before use. The npm packument (which carries dist.integrity) was
+        # unavailable, so refuse rather than loading unverified native code — mirroring
+        # the CLI download, which requires a checksum. Retry when the registry is
+        # reachable, or install a runtime package that ships the library.
+        raise RuntimeError(
+            "No Subresource Integrity value available for the in-process runtime "
+            f"library ({npm_platform}@{ver}); refusing to load unverified native code."
+        )
+    _verify_integrity(data, integrity)
 
     lib_bytes = _extract_runtime_node(data, npm_platform)
 
@@ -431,6 +445,8 @@ def ensure_runtime_library(cli_path: str, version: str | None = None) -> str | N
         try:
             os.unlink(tmp_name)
         except OSError:
+            # Best-effort cleanup of the temp file; ignore if it's already gone or
+            # can't be removed (the OS reclaims it, and it doesn't affect correctness).
             pass
         if lib_path.exists():
             return str(lib_path)

python/copilot/client.py

@@ -1419,7 +1419,16 @@ def __init__(
                 self._effective_connection_token = None
 
             # Resolve CLI path: explicit > COPILOT_CLI_PATH env var > downloaded binary.
-            effective_env = connection.env or options.env or os.environ
+            # Select the environment by identity, not truthiness, so an intentionally
+            # empty per-connection or client env stays authoritative (the spawned child
+            # receives that empty mapping) instead of falling back to os.environ and
+            # unexpectedly honoring a host COPILOT_CLI_PATH.
+            if connection.env is not None:
+                effective_env: Mapping[str, str] = connection.env
+            elif options.env is not None:
+                effective_env = options.env
+            else:
+                effective_env = os.environ
             connection.path = self._resolve_runtime_entrypoint(connection.path, env=effective_env)
 
             # Resolve use_logged_in_user default
@@ -3948,14 +3957,14 @@ async def _start_inprocess_ffi(self) -> None:
         cli_path = conn.path
         assert cli_path is not None  # resolved in __init__
 
-        # A packaged single-file CLI is invoked directly; a `.js` dev entrypoint is
-        # launched via node. Both are handled inside FfiRuntimeHost.
-        if not os.path.exists(cli_path) and cli_path.endswith(".js") is False:
-            resolved = shutil.which(cli_path)
-            if resolved is None:
-                raise RuntimeError(f"Copilot CLI not found at {cli_path}")
-            cli_path = resolved
-
+        # No PATH lookup here (unlike the child-process transport): the in-process
+        # entrypoint is always an absolute on-disk path — an explicit path,
+        # COPILOT_CLI_PATH, or the downloaded CLI — so the native library provisioned
+        # next to it in __init__ is exactly the one FfiRuntimeHost loads. Resolving a
+        # bare command name via PATH would look for the library beside a different
+        # directory than the one it was provisioned into and fail. A packaged
+        # single-file CLI is invoked directly; a `.js` dev entrypoint is launched via
+        # node — both handled inside FfiRuntimeHost.
         logger.info(
             "CopilotClient._start_inprocess_ffi hosting Copilot runtime in-process",
             extra={"cli_path": cli_path, "cli_path_source": self._cli_path_source},
@@ -3966,6 +3975,15 @@ async def _start_inprocess_ffi(self) -> None:
         # ambient environment. Pass extra worker args via connection.args.
         host = FfiRuntimeHost.create(cli_path, environment=None, args=conn.args)
 
+        # Track the host and expose its process-like adapter *before* the blocking
+        # handshake. asyncio.to_thread keeps running host_start after a cancellation
+        # (a thread can't be interrupted), and CancelledError bypasses start()'s
+        # `except Exception`, so assigning here — as .NET does before StartAsync —
+        # keeps a completed native host owned so stop()/force_stop() can dispose it
+        # instead of leaking it.
+        self._ffi_host = host
+        self._process = host.process
+
         ffi_start = time.perf_counter()
         # host_start blocks until the worker connects back and signals readiness,
         # so run the handshake off the event loop.
@@ -3977,11 +3995,6 @@ async def _start_inprocess_ffi(self) -> None:
             ffi_start,
         )
 
-        self._ffi_host = host
-        # Expose the process-like adapter so the JSON-RPC client and stop/force_stop
-        # paths treat it like a spawned runtime process.
-        self._process = host.process
-
     async def _connect_to_server(self) -> None:
         """Connect to the runtime via the configured transport.
 

python/test_cli_download.py

@@ -0,0 +1,53 @@
+"""Tests for the in-process runtime library download integrity checks."""
+
+from __future__ import annotations
+
+import base64
+import hashlib
+from unittest.mock import patch
+
+import pytest
+
+from copilot import _cli_download
+
+
+def _integrity(data: bytes, algo: str = "sha512") -> str:
+    digest = hashlib.new(algo, data).digest()
+    return f"{algo}-{base64.b64encode(digest).decode('ascii')}"
+
+
+class TestVerifyIntegrity:
+    def test_accepts_matching_checksum(self):
+        data = b"native-library-bytes"
+        _cli_download._verify_integrity(data, _integrity(data))
+
+    def test_rejects_mismatched_checksum(self):
+        with pytest.raises(RuntimeError, match="Integrity mismatch"):
+            _cli_download._verify_integrity(b"tampered", _integrity(b"original"))
+
+    def test_rejects_unsupported_algorithm(self):
+        # Fail closed rather than silently skipping verification of native code.
+        with pytest.raises(RuntimeError, match="Unsupported integrity algorithm"):
+            _cli_download._verify_integrity(b"bytes", "md5-deadbeef")
+
+
+class TestEnsureRuntimeLibraryFailsClosed:
+    def test_raises_when_integrity_unavailable(self, tmp_path):
+        """A missing npm integrity value must abort the download, not load unverified code."""
+        cli_path = tmp_path / "copilot"
+        cli_path.write_bytes(b"#!/bin/sh\n")
+
+        with (
+            patch("copilot._ffi_runtime_host.resolve_library_path", return_value=None),
+            patch.object(_cli_download, "_should_skip_download", return_value=False),
+            patch.object(_cli_download, "get_npm_platform", return_value="linux-x64"),
+            patch.object(_cli_download, "get_runtime_lib_url", return_value="https://example/lib"),
+            patch.object(_cli_download, "_fetch_url_bytes", return_value=b"tarball-bytes"),
+            patch.object(_cli_download, "_fetch_runtime_integrity", return_value=None),
+            patch.object(_cli_download, "_extract_runtime_node") as extract,
+        ):
+            with pytest.raises(RuntimeError, match="refusing to load unverified native code"):
+                _cli_download.ensure_runtime_library(str(cli_path), version="1.2.3")
+
+        # The library bytes must never be extracted/written when verification is impossible.
+        extract.assert_not_called()