Address review comments: Final annotation, atexit tmp cleanup, parametrized tests

- Annotate DEFAULT_COANA_CLI_VERSION with typing.Final.
- Register an atexit handler to remove the npm-install fallback's temp dirs.
- Trim the over-long --force explanation in _spawn_coana's docstring and drop the
  inline comment that duplicated it.
- Use try/finally in the cache-clearing test fixture.
- Parametrize the spec-resolution, npx-version, and launcher-failure-heuristic tests.

Author: mtorp

socketsecurity/core/tools/reachability.py

@@ -1,7 +1,9 @@
 from socketdev import socketdev
-from typing import List, Optional, Dict, Any
+from typing import List, Optional, Dict, Any, Final
+import atexit
 import os
 import platform
+import shutil
 import subprocess
 import json
 import pathlib
@@ -16,13 +18,22 @@
 # Pinned @coana-tech/cli version. Bumped deliberately per Python CLI release so the
 # reachability engine version only changes through a standard pip upgrade (advance notice).
 # Pass --reach-version latest to opt into the newest published version instead.
-DEFAULT_COANA_CLI_VERSION = "15.3.24"
+DEFAULT_COANA_CLI_VERSION: Final = "15.3.24"
 
 # Resolved @coana-tech/cli script paths from the npm-install fallback, keyed by version.
 # Lives for the process lifetime so repeated fallback invocations install only once
 # (mirrors the Node CLI's installedCoanaScriptPathsByVersion).
 _INSTALLED_COANA_SCRIPT_PATHS: Dict[str, str] = {}
 
+# Temp dirs created by the npm-install fallback, removed at process exit.
+_COANA_INSTALL_DIRS: List[str] = []
+
+
+@atexit.register
+def _cleanup_coana_install_dirs() -> None:
+    for install_dir in _COANA_INSTALL_DIRS:
+        shutil.rmtree(install_dir, ignore_errors=True)
+
 
 def _build_caller_user_agent() -> str:
     """Build the SOCKET_CALLER_USER_AGENT string forwarded to the coana CLI.
@@ -302,12 +313,9 @@ def _spawn_coana(
     ) -> int:
         """Run coana for the given args, returning the process exit code.
 
-        Primary path: ``npx --yes --force @coana-tech/cli@<version> ...`` — the exact flags
-        the Socket Node CLI passes for coana. ``--yes`` skips npx's interactive install
-        confirmation so non-interactive/CI runs don't hang. ``--force`` matches the Node CLI
-        (it opts out of npm's prompts/protections and refreshes within-range specs); note it
-        does NOT force a re-download of an already-cached pinned version — npx still reuses a
-        cached pinned package, so this is parity with the Node CLI, not a cache bypass.
+        Primary path: ``npx --yes --force @coana-tech/cli@<version> ...`` — the exact flags the
+        Socket Node CLI passes for coana (``--yes`` skips npx's interactive install prompt so
+        CI runs don't hang).
 
         Fallback path: if npx is missing, or its launcher dies before coana starts, install
         @coana-tech/cli into a temp dir via ``npm install`` and run it directly via ``node``.
@@ -322,7 +330,6 @@ def _spawn_coana(
             return self._spawn_coana_via_npm_install(coana_args, effective_version, coana_env, cwd)
 
         package_spec = f"@coana-tech/cli@{effective_version}"
-        # --yes skips npx's install confirmation; --force matches the Node CLI's coana flags.
         npx_cmd = ["npx", "--yes", "--force", package_spec, *coana_args]
         log.debug(f"Reachability command: {' '.join(npx_cmd)}")
         try:
@@ -390,6 +397,7 @@ def _install_coana_to_tmpdir(self, version: str, env: Dict[str, str]) -> str:
             return cached
 
         install_dir = tempfile.mkdtemp(prefix="socket-coana-")
+        _COANA_INSTALL_DIRS.append(install_dir)
         npm_cmd = [
             "npm", "install",
             "--no-save", "--no-package-lock", "--no-audit", "--no-fund",

tests/unit/test_reachability.py

@@ -28,8 +28,10 @@ def analyzer():
 def _clear_coana_install_cache():
     """The npm-install fallback caches resolved script paths in a module-level dict; isolate tests."""
     reachability._INSTALLED_COANA_SCRIPT_PATHS.clear()
-    yield
-    reachability._INSTALLED_COANA_SCRIPT_PATHS.clear()
+    try:
+        yield
+    finally:
+        reachability._INSTALLED_COANA_SCRIPT_PATHS.clear()
 
 
 def _spawn_mock(analyzer, mocker, returncode=0, **kwargs):
@@ -125,27 +127,21 @@ def test_repo_branch_env_absent_when_none(analyzer, mocker):
 # --- Coana package-spec resolution (pinned by default, latest is opt-in) ---
 
 
-def test_resolve_spec_defaults_to_pinned_version(analyzer):
-    """No --reach-version -> pinned DEFAULT_COANA_CLI_VERSION (no auto-update)."""
-    assert (
-        analyzer._resolve_coana_package_spec(None)
-        == f"@coana-tech/cli@{DEFAULT_COANA_CLI_VERSION}"
-    )
-
-
-def test_resolve_spec_pins_explicit_version(analyzer):
-    assert analyzer._resolve_coana_package_spec("1.2.3") == "@coana-tech/cli@1.2.3"
-
-
-def test_resolve_spec_latest_opt_in(analyzer):
-    """'latest' opts into the newest published version."""
-    assert analyzer._resolve_coana_package_spec("latest") == "@coana-tech/cli@latest"
-
-
-def test_resolve_spec_is_always_versioned(analyzer):
-    """Never the bare '@coana-tech/cli' (which would let npx pick a stray global version)."""
-    for version in (None, "latest", "1.2.3", " 1.2.3 "):
-        assert analyzer._resolve_coana_package_spec(version).startswith("@coana-tech/cli@")
+@pytest.mark.parametrize(
+    ("version", "expected"),
+    [
+        # No --reach-version -> pinned default (no auto-update).
+        (None, f"@coana-tech/cli@{DEFAULT_COANA_CLI_VERSION}"),
+        # Explicit version pinned through.
+        ("1.2.3", "@coana-tech/cli@1.2.3"),
+        # 'latest' opts into the newest published version.
+        ("latest", "@coana-tech/cli@latest"),
+        # Surrounding whitespace is stripped; always versioned (never bare '@coana-tech/cli').
+        (" 1.2.3 ", "@coana-tech/cli@1.2.3"),
+    ],
+)
+def test_resolve_coana_package_spec(analyzer, version, expected):
+    assert analyzer._resolve_coana_package_spec(version) == expected
 
 
 def _spec_in(cmd):
@@ -161,19 +157,17 @@ def test_npx_uses_yes_and_force_flags(analyzer, mocker):
     assert "--force" in cmd
 
 
-def test_npx_runs_pinned_version_by_default(analyzer, mocker):
-    cmd, _ = _run(analyzer, mocker)
-    assert _spec_in(cmd) == f"@coana-tech/cli@{DEFAULT_COANA_CLI_VERSION}"
-
-
-def test_npx_runs_explicit_version(analyzer, mocker):
-    cmd, _ = _run(analyzer, mocker, version="9.9.9")
-    assert _spec_in(cmd) == "@coana-tech/cli@9.9.9"
-
-
-def test_npx_runs_latest_when_opted_in(analyzer, mocker):
-    cmd, _ = _run(analyzer, mocker, version="latest")
-    assert _spec_in(cmd) == "@coana-tech/cli@latest"
+@pytest.mark.parametrize(
+    ("version", "expected_spec"),
+    [
+        (None, f"@coana-tech/cli@{DEFAULT_COANA_CLI_VERSION}"),  # pinned default
+        ("9.9.9", "@coana-tech/cli@9.9.9"),                     # explicit pin
+        ("latest", "@coana-tech/cli@latest"),                  # opt-in to newest
+    ],
+)
+def test_npx_runs_resolved_version(analyzer, mocker, version, expected_spec):
+    cmd, _ = _run(analyzer, mocker, version=version)
+    assert _spec_in(cmd) == expected_spec
 
 
 def test_default_path_never_runs_npm_install(analyzer, mocker):
@@ -195,16 +189,22 @@ def test_env_strips_npm_package_vars(analyzer, mocker, monkeypatch):
 # --- npm-install + node fallback (when the npx launcher fails before coana starts) ---
 
 
-def test_launcher_failure_heuristic():
+@pytest.mark.parametrize(
+    ("returncode", "is_launcher_failure"),
+    [
+        # Signal kills / >=128 -> launcher failure -> retry.
+        (-9, True),    # killed by signal
+        (137, True),   # 128 + SIGKILL
+        (249, True),   # observed npx launcher failure
+        # Small positive exit codes are ambiguous (coana's own codes) -> do NOT retry.
+        (1, False),
+        (2, False),
+        (127, False),
+    ],
+)
+def test_launcher_failure_heuristic(returncode, is_launcher_failure):
     f = ReachabilityAnalyzer._npx_launcher_failed_before_coana
-    # Signal kills / >=128 -> launcher failure -> retry.
-    assert f(-9) is True
-    assert f(137) is True
-    assert f(249) is True
-    # Small positive exit codes are ambiguous (coana's own codes) -> do NOT retry.
-    assert f(1) is False
-    assert f(2) is False
-    assert f(127) is False
+    assert f(returncode) is is_launcher_failure
 
 
 def _capture_spawns(analyzer, mocker, npx_behavior, **kwargs):