fix: verify the downloaded browser can resolve its libraries

src/kiro_crew/browser_cli/install.py

@@ -702,6 +702,62 @@ def _step(
     }
 
 
+def _engine_cache_dirs(engine: str) -> list[Path]:
+    """Cache directories holding builds for *engine*.
+
+    Prefix-matched rather than composed from a revision, so the CHROMIUM entry
+    also picks up ``chromium_headless_shell-<rev>``. That is not a bonus: headless
+    is the default launch mode, so the headless shell is the binary a browse
+    actually starts, and a library check that looked only at ``chromium-<rev>``
+    would clear the build that is not the one being run.
+    """
+    cache = _browsers_cache_dir()
+    if cache is None:
+        return []
+    try:
+        return [
+            child
+            for child in cache.iterdir()
+            if child.is_dir() and child.name.startswith(engine)
+        ]
+    except OSError:
+        return []
+
+
+def _verify_browser_libraries(engine: str) -> dict[str, Any] | None:
+    """Step reporting that *engine*'s downloaded build cannot resolve its libraries.
+
+    ``None`` when there is nothing to report -- no missing library, or the probe
+    could not run (see :func:`os_deps.missing_shared_libraries`). An unknown must
+    not fail an install: the browser download itself needs no privilege, and
+    turning "could not check" into "broken" would withdraw a working browser.
+
+    A step of its own rather than flipping the download's verdict, because the
+    download genuinely SUCCEEDED. Labelling it failed would send the operator to
+    re-download bytes that are already on disk, when what they need is root and a
+    package manager.
+    """
+    missing = os_deps.missing_shared_libraries(_engine_cache_dirs(engine))
+    if not missing:
+        return None
+    listed = ", ".join(sorted(missing))
+    hint = os_deps.missing_deps_hint()
+    detail = (
+        f"{engine} downloaded, but {len(missing)} shared "
+        f"librar{'y' if len(missing) == 1 else 'ies'} cannot be resolved on this "
+        f"host, so the browser cannot launch: {listed}"
+    )
+    return {
+        "name": f"verify-browser-libraries-{engine}",
+        "ok": False,
+        # Nothing exited non-zero -- the download really did succeed. Reported
+        # honestly rather than borrowing a failure code from a process that
+        # never ran.
+        "returncode": 0,
+        "stderr": f"{detail}\n\n{hint}" if hint else detail,
+    }
+
+
 def _download_browser(path: str, engine: str | None = None) -> list[dict[str, Any]]:
     """Download a browser build, adapting to what this host's OS allows.
 
@@ -718,7 +774,10 @@ def _download_browser(path: str, engine: str | None = None) -> list[dict[str, An
     tried instead of only the last verdict.
 
     Every attempt is judged on its output as well as its exit code: a build whose
-    libraries are missing downloads "successfully" and cannot launch.
+    libraries are missing downloads "successfully" and cannot launch. That check
+    is not sufficient by itself either -- Playwright's validation false-negatives
+    on linux-arm64 -- so a successful download is FOLLOWED by a real library
+    probe (:func:`_verify_browser_libraries`).
     """
     selected_engine = engine or _DEFAULT_BROWSER_ENGINE
     base = [path, "install-browser", selected_engine]
@@ -736,13 +795,20 @@ def attempt(step_name: str, argv: list[str], with_hint: bool) -> dict[str, Any]:
             failure_signal=os_deps.host_deps_unsatisfied,
         )
 
+    def with_library_check(steps: list[dict[str, Any]]) -> list[dict[str, Any]]:
+        """Append the library verdict when the download succeeded."""
+        if not steps[-1]["ok"]:
+            return steps
+        verdict = _verify_browser_libraries(selected_engine)
+        return steps if verdict is None else [*steps, verdict]
+
     if not os_deps.with_deps_supported():
-        return [attempt(f"install-browser{suffix}", base, True)]
+        return with_library_check([attempt(f"install-browser{suffix}", base, True)])
 
     first = attempt(f"install-browser{suffix}", base + ["--with-deps"], False)
     if first["ok"]:
-        return [first]
-    return [first, attempt(f"install-browser{suffix}-no-deps", base, True)]
+        return with_library_check([first])
+    return with_library_check([first, attempt(f"install-browser{suffix}-no-deps", base, True)])
 
 
 def install() -> dict[str, Any]:

src/kiro_crew/browser_cli/os_deps.py

@@ -16,13 +16,23 @@
 
 The read blocks (the os-release file), so a caller on the event loop offloads
 them -- the same contract as the rest of this package.
+
+This module also owns the check that the downloaded browser can actually RESOLVE
+its libraries (:func:`missing_shared_libraries`), because Playwright's own host
+validation cannot be relied on to say so -- on linux-arm64 it scans a directory
+that does not exist and concludes the host is fine. See that function.
 """
 
 from __future__ import annotations
 
 import logging
+import os
 import platform
+import subprocess
+import time
+from collections.abc import Iterable
 from functools import lru_cache
+from pathlib import Path
 
 from kiro_crew import platform_compat
 
@@ -234,6 +244,189 @@ def host_deps_unsatisfied(text: str) -> bool:
 
     Read the exit code AND this, never the exit code alone -- see
     :data:`_HOST_VALIDATION_MARKERS` for the measurement.
+
+    NOT sufficient on its own either: Playwright's validation produces FALSE
+    NEGATIVES on linux-arm64, so a host with libraries missing can emit no
+    marker at all. :func:`missing_shared_libraries` is the check that does not
+    depend on Playwright noticing.
     """
     lowered = (text or "").lower()
     return any(marker in lowered for marker in _HOST_VALIDATION_MARKERS)
+
+
+#: Ceiling on the ELF files probed per browser directory. MEASURED: chromium
+#: ships 14 (9 executables + 5 shared objects) and firefox 44, so the real
+#: counts sit far below this -- it exists to bound a future layout, not to clip
+#: today's.
+_LDD_MAX_FILES = 200
+#: ``ldd`` on one file MEASURED at ~9 ms, so a whole browser directory is well
+#: under a second. The budget covers the whole sweep, not one file.
+_LDD_TIMEOUT_S = 60.0
+
+#: ``ldd`` marks an unresolvable dependency with this. Matched on the line rather
+#: than parsed: the left-hand side is the soname we want to report and the rest
+#: of the line is formatting.
+_LDD_NOT_FOUND = "not found"
+
+
+def _library_search_dirs(directories: Iterable[Path]) -> list[Path]:
+    """Directories inside the build that hold its own shared objects.
+
+    Fed to ``ldd`` as ``LD_LIBRARY_PATH`` because a browser ships libraries it
+    loads from beside itself, and ``ldd`` run with a bare environment cannot
+    resolve them. MEASURED: without this, firefox reports ``libxul.so``,
+    ``liblgpllibs.so``, ``libmozsqlite3.so`` and 7 more as missing while the
+    browser launches perfectly -- so the check would have blocked a WORKING
+    install, a worse failure than the one it exists to catch. Playwright passes
+    its own directory list for exactly this reason.
+    """
+    dirs: set[Path] = set()
+    for directory in directories:
+        try:
+            for path in directory.rglob("*.so*"):
+                if path.is_file():
+                    dirs.add(path.parent)
+        except OSError:
+            continue
+    return sorted(dirs)
+
+
+def _sonames_shipped_with_build(directories: Iterable[Path]) -> set[str]:
+    """File names of shared objects the build carries itself.
+
+    A second, independent guard behind ``LD_LIBRARY_PATH``: a library present in
+    the tree is the build's own, so naming it "missing on this host" is wrong
+    regardless of whether ``ldd`` managed to resolve it. Keeps a loader quirk
+    (``$ORIGIN`` handling, a nested layout the search path missed) from
+    resurrecting the false positive above.
+    """
+    names: set[str] = set()
+    for directory in directories:
+        try:
+            for path in directory.rglob("*.so*"):
+                if path.is_file():
+                    names.add(path.name)
+        except OSError:
+            continue
+    return names
+
+
+def _elf_candidates(directory: Path) -> list[Path]:
+    """Executables and shared objects under *directory*, most-important first.
+
+    Sorted so the main program is probed before its satellites when the ceiling
+    truncates: an executable outranks a ``.so``, and a shallower path outranks a
+    deeper one. Without an order the ceiling would drop an arbitrary subset,
+    which is how a check reports "no missing libraries" for a browser whose main
+    binary was never looked at.
+    """
+    found: list[Path] = []
+    for path in sorted(directory.rglob("*")):
+        try:
+            if not path.is_file() or path.is_symlink():
+                continue
+            is_executable = os.access(path, os.X_OK)
+            is_shared_object = ".so" in path.name
+            if is_executable or is_shared_object:
+                found.append(path)
+        except OSError:
+            continue
+    found.sort(key=lambda p: (not os.access(p, os.X_OK), len(p.parts), str(p)))
+    return found[:_LDD_MAX_FILES]
+
+
+def _missing_sonames_in_output(stdout: str, shipped: set[str]) -> set[str]:
+    """Unresolvable sonames in one ``ldd`` output, excluding the build's own.
+
+    ``ldd`` prefixes its report with ``<path>:`` when it has something to say
+    about the file itself; that header ends in a colon and is not a soname. Left
+    unfiltered it was reported as a missing library named after an absolute path
+    -- nonsense in the operator's remedy, and it named a file that is present.
+    """
+    missing: set[str] = set()
+    for line in stdout.splitlines():
+        stripped = line.strip()
+        if _LDD_NOT_FOUND not in stripped or stripped.endswith(":"):
+            continue
+        fields = stripped.split()
+        if not fields:
+            continue
+        soname = fields[0]
+        if soname.endswith(":") or "/" in soname or soname in shipped:
+            continue
+        missing.add(soname)
+    return missing
+
+
+def missing_shared_libraries(directories: Iterable[Path]) -> set[str] | None:
+    """Sonames the downloaded browser needs that this host cannot resolve.
+
+    Exists because Playwright's own host validation CANNOT be trusted to notice.
+    MEASURED on Amazon Linux 2023 arm64: its registry declares the directory to
+    scan as ``chrome-linux``, while the arm64 build unpacks into
+    ``chrome-linux-arm64``, so it scans a path that does not exist, finds no
+    dependencies at all, concludes the host is fine and writes its
+    ``DEPENDENCIES_VALIDATED`` marker -- which then suppresses re-validation for
+    30 days. The marker was written 23 minutes BEFORE the libraries were
+    installed on that host. A false negative, not a missing warning: the
+    download reports success, ``browser_ok`` reports true, the panel goes green,
+    and the failure only arrives at the user's first browse as an opaque stack
+    trace.
+
+    So this reads the real files, and never hardcodes a build's subdirectory
+    name -- that assumption is the upstream bug being worked around.
+
+    Only libraries the HOST must provide are reported; the build's own bundled
+    ones are excluded twice over (see :func:`_library_search_dirs` and
+    :func:`_sonames_shipped_with_build`), because reporting those would block an
+    install that works.
+
+    ``None`` means "cannot determine", NOT "nothing missing": off Linux (no
+    ``ldd``), when no directory exists, or when ``ldd`` itself is unavailable or
+    times out. Callers must not turn an unknown into a failed install -- an
+    absent probe is not evidence of a broken browser.
+    """
+    if not platform_compat.IS_LINUX:
+        return None
+    dirs = [d for d in directories]
+    candidates: list[Path] = []
+    for directory in dirs:
+        try:
+            if directory.is_dir():
+                candidates.extend(_elf_candidates(directory))
+        except OSError:
+            continue
+    if not candidates:
+        return None
+    shipped = _sonames_shipped_with_build(dirs)
+    env = dict(os.environ)
+    search = os.pathsep.join(str(d) for d in _library_search_dirs(dirs))
+    if search:
+        existing = env.get("LD_LIBRARY_PATH", "")
+        env["LD_LIBRARY_PATH"] = f"{search}{os.pathsep}{existing}" if existing else search
+    missing: set[str] = set()
+    deadline = time.monotonic() + _LDD_TIMEOUT_S
+    probed = 0
+    for path in candidates:
+        if time.monotonic() >= deadline:
+            logger.warning("ldd sweep timed out after %d files", probed)
+            break
+        try:
+            proc = subprocess.run(  # noqa: S603 - fixed argv, path from our own cache
+                ["ldd", str(path)],
+                capture_output=True,
+                text=True,
+                timeout=max(1.0, deadline - time.monotonic()),
+                check=False,
+                env=env,
+            )
+        except (OSError, subprocess.TimeoutExpired):
+            # A file ldd refuses (not an ELF, wrong class) is not evidence of a
+            # missing library. Skip it rather than letting it fail the host.
+            continue
+        probed += 1
+        missing |= _missing_sonames_in_output(proc.stdout or "", shipped)
+    if not probed:
+        # Every probe failed -- most likely ldd is absent. Unknown, not clean.
+        return None
+    return missing