Puppeteer/Playwright: collapse browser_cache_dir → single writable cache_dir

claude · claude · commit 2d52f0bbe585 · 2026-04-21T22:24:47.000Z
Per maintainer feedback (#33): providers should expose only ``install_root``, ``cache_dir``, and ``bin_dir`` — no duplicate ``browser_cache_dir`` field. Refactor to match: - Drop the separate ``browser_cache_dir`` field on both providers. - Promote ``cache_dir`` from a computed property to a writable field with a ``default_factory`` that hydrates from ``PUPPETEER_CACHE_DIR`` / ``PLAYWRIGHT_BROWSERS_PATH``. - A ``@model_validator(mode="after")`` on each provider fills ``cache_dir = install_root/cache`` when ``cache_dir`` is still ``None`` but an ``install_root`` is pinned (preserving the previous default). - Precedence at validate time: 1. explicit ``cache_dir`` kwarg wins 2. else env var (PUPPETEER_CACHE_DIR / PLAYWRIGHT_BROWSERS_PATH) 3. else ``<install_root>/cache`` when an install root is pinned 4. else ``None`` (global / OS default) Uninstall + abspath checks: - ``PlaywrightProvider.default_uninstall_handler`` now iterates the resolved ``cache_dir`` (= ``PLAYWRIGHT_BROWSERS_PATH`` tree) instead of ``install_root`` when cleaning ``<browser>-*/`` dirs. - ``PlaywrightProvider.default_abspath_handler`` now scopes ``executablePath()`` hits to ``cache_dir.resolve()`` ancestry instead of ``install_root`` ancestry, so an explicit ``PLAYWRIGHT_BROWSERS_PATH`` outside ``install_root`` still satisfies ``load()``. - ``PuppeteerProvider`` already routed all cache ops through ``self.cache_dir`` — no behaviour change there, just field cleanup. README updated to reflect the collapsed API surface for both providers.
diff --git a/README.md b/README.md
@@ -1074,11 +1074,11 @@ INSTALLER_BIN = "puppeteer-browsers"
 PATH = ""
 install_root = $ABXPKG_PUPPETEER_ROOT or $ABXPKG_LIB_DIR/puppeteer
 bin_dir = <install_root>/bin
-browser_cache_dir = None          # hydrated from PUPPETEER_CACHE_DIR env; overrides install_root/cache
+cache_dir = <install_root>/cache  # hydrated from PUPPETEER_CACHE_DIR env when install_root is unset
 ```
 
 - Install root: set `install_root` for the root dir and `bin_dir` for symlinked executables. Leave it unset for ambient/global mode, where cache ownership stays with the host and `INSTALLER_BINARY` must already be resolvable from the ambient provider set.
-- Cache dir / `PUPPETEER_CACHE_DIR`: browser downloads land in the directory resolved with three-tier precedence: explicit `browser_cache_dir` / `PUPPETEER_CACHE_DIR` env wins → else `<install_root>/cache` when an install root is pinned → else `None`. The resolved value is exported as `PUPPETEER_CACHE_DIR` to every subprocess the provider runs.
+- Cache dir / `PUPPETEER_CACHE_DIR`: browser downloads land in `cache_dir`, resolved with four-tier precedence at validate time: explicit `cache_dir` kwarg wins → else `PUPPETEER_CACHE_DIR` env var → else `<install_root>/cache` when an install root is pinned → else `None`. The resolved value is exported as `PUPPETEER_CACHE_DIR` to every subprocess the provider runs.
 - Auto-switching: bootstraps `@puppeteer/browsers` through `NpmProvider` and then uses that CLI for browser installs.
 - `dry_run`: shared behavior.
 - Security: `min_release_age` is unsupported for browser installs and is ignored with a warning if explicitly requested. `postinstall_scripts=False` is supported for the underlying npm bootstrap path, and `ABXPKG_POSTINSTALL_SCRIPTS` hydrates the provider default here.
@@ -1097,12 +1097,12 @@ INSTALLER_BIN = "playwright"
 PATH = ""
 install_root = None              # abxpkg-managed root dir for bin_dir / nested npm prefix
 bin_dir = <install_root>/bin     # symlink dir for resolved browsers
-browser_cache_dir = None         # hydrated from PLAYWRIGHT_BROWSERS_PATH env; overrides install_root/cache
+cache_dir = <install_root>/cache # hydrated from PLAYWRIGHT_BROWSERS_PATH env when install_root is unset
 euid = 0                         # routes exec() through sudo-first-then-fallback
 ```
 
 - Install root: set `install_root` to pin the abxpkg-managed root dir (where `bin_dir` symlinks and the nested npm prefix live). Leave it unset to let playwright use its own OS-default browsers path (`~/.cache/ms-playwright` on Linux etc.) — in that case abxpkg maintains no symlink dir or npm prefix at all, the `playwright` npm CLI bootstraps against the host's npm default, and `load()` returns the resolved `executablePath()` directly. `bin_dir` overrides the symlink directory when `install_root` is pinned.
-- Cache dir / `PLAYWRIGHT_BROWSERS_PATH`: the actual browsers cache dir is resolved via the `cache_dir` computed property with three-tier precedence: explicit `browser_cache_dir` / `PLAYWRIGHT_BROWSERS_PATH` env wins → else `<install_root>/cache` when an install root is pinned → else `None` (let playwright pick its OS default). The final value is exported as `PLAYWRIGHT_BROWSERS_PATH` to every subprocess the provider runs.
+- Cache dir / `PLAYWRIGHT_BROWSERS_PATH`: browser downloads land in `cache_dir`, resolved with four-tier precedence at validate time: explicit `cache_dir` kwarg wins → else `PLAYWRIGHT_BROWSERS_PATH` env var → else `<install_root>/cache` when an install root is pinned → else `None` (let playwright pick its OS default). The final value is exported as `PLAYWRIGHT_BROWSERS_PATH` to every subprocess the provider runs. `uninstall()` deletes matching `<browser>-*/` dirs from the resolved `cache_dir` (never from playwright's untouched OS-default cache when nothing is pinned).
 - Auto-switching: bootstraps the `playwright` npm package through `NpmProvider`, then runs `playwright install --with-deps <install_args>` against it. Resolves each installed browser's real executable via the `playwright-core` Node.js API (`chromium.executablePath()` etc.) and writes a symlink into `bin_dir` when one is configured.
 - `dry_run`: shared behavior — the install handler short-circuits to a placeholder without touching the host.
 - Privilege handling: `--with-deps` installs system packages and requires root on Linux. ``euid`` defaults to ``0``, which routes every ``exec()`` call through the base ``BinProvider.exec`` sudo-first-then-fallback path — it tries ``sudo -n -- playwright install --with-deps ...`` first on non-root hosts, falls back to running the command directly if sudo fails or isn't available, and merges both stderr outputs into the final error if both attempts fail.
diff --git a/abxpkg/binprovider_playwright.py b/abxpkg/binprovider_playwright.py
@@ -8,8 +8,9 @@
 import sys
 import platform
 from pathlib import Path
+from typing import Self
 
-from pydantic import Field, TypeAdapter, computed_field
+from pydantic import Field, TypeAdapter, computed_field, model_validator
 
 from .base_types import (
     BinName,
@@ -72,14 +73,14 @@ class PlaywrightProvider(BinProvider):
     # Only set in managed mode: setup()/default_abspath_handler() use it to create and read
     # stable browser shims under ``<install_root>/bin``; global mode leaves it unset.
     bin_dir: Path | None = None
-    # Explicit override for the directory browsers get downloaded into.
-    # Hydrated from ``PLAYWRIGHT_BROWSERS_PATH`` (playwright's own standard
-    # env var) when unset so callers can pin the cache dir the same way they
-    # would for a vanilla ``playwright install``. When both this and
-    # ``install_root`` are unset, playwright falls back to its OS default; when
-    # only ``install_root`` is set, ``cache_dir`` defaults to
-    # ``<install_root>/cache`` (see ``ENV``).
-    browser_cache_dir: Path | None = Field(
+    # Where browsers get downloaded. Exported as ``PLAYWRIGHT_BROWSERS_PATH``
+    # to every subprocess. Precedence at validate time:
+    #   1. explicit ``cache_dir`` kwarg wins (more-specific override)
+    #   2. else ``PLAYWRIGHT_BROWSERS_PATH`` env var wins (same semantics
+    #      for external callers invoking via env)
+    #   3. else ``<install_root>/cache`` when an install root is pinned
+    #   4. else ``None`` (global mode — let playwright pick its OS default)
+    cache_dir: Path | None = Field(
         default_factory=lambda: (
             Path(os.environ["PLAYWRIGHT_BROWSERS_PATH"]).expanduser()
             if os.environ.get("PLAYWRIGHT_BROWSERS_PATH")
@@ -92,32 +93,18 @@ class PlaywrightProvider(BinProvider):
     # run as the normal user by default.
     euid: int | None = 0 if platform.system().lower() == "linux" else None
 
+    @model_validator(mode="after")
+    def _fill_cache_dir_from_install_root(self) -> Self:
+        if self.cache_dir is None and self.install_root is not None:
+            self.cache_dir = self.install_root / "cache"
+        return self
+
     @computed_field
     @property
     def ENV(self) -> "dict[str, str]":
-        resolved = self.cache_dir
-        if resolved is None:
+        if not self.cache_dir:
             return {}
-        return {"PLAYWRIGHT_BROWSERS_PATH": str(resolved)}
-
-    @computed_field
-    @property
-    def cache_dir(self) -> Path | None:
-        """PLAYWRIGHT_BROWSERS_PATH precedence:
-
-        1. Explicit ``browser_cache_dir`` (field / ``PLAYWRIGHT_BROWSERS_PATH`` env)
-           wins — the more specific cache-dir override always beats the less
-           specific install-root.
-        2. Else, when ``install_root`` is pinned, default to
-           ``<install_root>/cache`` so managed installs stay self-contained.
-        3. Else leave it unset and let playwright pick its own OS-default
-           browsers cache directory.
-        """
-        if self.browser_cache_dir is not None:
-            return self.browser_cache_dir
-        if self.install_root is not None:
-            return self.install_root / "cache"
-        return None
+        return {"PLAYWRIGHT_BROWSERS_PATH": str(self.cache_dir)}
 
     def supports_min_release_age(self, action, no_cache: bool = False) -> bool:
         return False
@@ -605,14 +592,15 @@ def default_abspath_handler(
         )
         if not resolved:
             return None
-        # When ``playwright_root`` is pinned, a hit from
-        # ``executablePath()`` that points outside that install_root tree
-        # (e.g. an ambient system install) should not satisfy
-        # ``load()`` — otherwise an unrelated host-wide playwright
-        # install would silently hijack resolution.
-        if self.install_root is not None:
-            root_real = self.install_root.resolve(strict=False)
-            if root_real not in resolved.resolve(strict=False).parents:
+        # When ``cache_dir`` is pinned (either explicitly, via
+        # ``PLAYWRIGHT_BROWSERS_PATH``, or derived from ``install_root``),
+        # an ``executablePath()`` hit that points outside that tree
+        # (e.g. an ambient system install) should not satisfy ``load()``
+        # — otherwise an unrelated host-wide playwright install would
+        # silently hijack resolution.
+        if self.cache_dir is not None:
+            cache_real = self.cache_dir.resolve(strict=False)
+            if cache_real not in resolved.resolve(strict=False).parents:
                 return None
         if self.bin_dir is None:
             return resolved
@@ -763,11 +751,13 @@ def default_uninstall_handler(
             (self.bin_dir / bin_name).unlink(missing_ok=True)
 
         # ``playwright uninstall`` only removes *unused* browsers from
-        # the entire host, so drop the matching directories ourselves.
-        # Only touch ``playwright_root`` if the caller pinned one — we
-        # don't delete from playwright's own OS-default cache.
-        if self.install_root is not None and self.install_root.is_dir():
-            for entry in self.install_root.iterdir():
+        # the entire host, so drop the matching directories ourselves
+        # from the resolved ``cache_dir`` (= ``PLAYWRIGHT_BROWSERS_PATH``).
+        # Only touch it if the caller pinned one explicitly or via
+        # ``install_root`` — we don't delete from playwright's own
+        # OS-default cache.
+        if self.cache_dir is not None and self.cache_dir.is_dir():
+            for entry in self.cache_dir.iterdir():
                 if entry.is_dir() and entry.name.startswith(f"{bin_name}-"):
                     logger.info("$ %s", format_command(["rm", "-rf", str(entry)]))
                     shutil.rmtree(entry, ignore_errors=True)
diff --git a/abxpkg/binprovider_puppeteer.py b/abxpkg/binprovider_puppeteer.py
@@ -66,12 +66,14 @@ class PuppeteerProvider(BinProvider):
     # Only set in managed mode: setup()/default_abspath_handler() use it to expose stable
     # browser launch shims under ``<install_root>/bin``; global mode leaves it unset.
     bin_dir: Path | None = None
-    # Explicit override for the directory browsers get downloaded into.
-    # Hydrated from ``PUPPETEER_CACHE_DIR`` (puppeteer's own standard env var)
-    # when unset so callers can pin the cache dir the same way they would for
-    # a vanilla ``puppeteer-browsers install``. When still unset, ``cache_dir``
-    # falls back to ``<install_root>/cache``.
-    browser_cache_dir: Path | None = Field(
+    # Where browsers get downloaded. Precedence at validate time:
+    #   1. explicit ``cache_dir`` kwarg wins (more-specific override)
+    #   2. else ``PUPPETEER_CACHE_DIR`` env var wins (same semantics for
+    #      external callers invoking via env)
+    #   3. else ``<install_root>/cache`` when an install root is pinned
+    #   4. else ``None`` (global mode — let puppeteer-browsers pick its
+    #      own default cache)
+    cache_dir: Path | None = Field(
         default_factory=lambda: (
             Path(os.environ["PUPPETEER_CACHE_DIR"]).expanduser()
             if os.environ.get("PUPPETEER_CACHE_DIR")
@@ -89,22 +91,12 @@ def ENV(self) -> "dict[str, str]":
     def supports_postinstall_disable(self, action, no_cache: bool = False) -> bool:
         return action in ("install", "update")
 
-    @computed_field
-    @property
-    def cache_dir(self) -> Path | None:
-        # Explicit override wins so PUPPETEER_CACHE_DIR can be any path.
-        if self.browser_cache_dir is not None:
-            return self.browser_cache_dir
-        # Otherwise browser downloads live under ``install_root/cache`` when we
-        # manage an install root; global mode leaves cache ownership to the host.
-        if self.install_root is not None:
-            return self.install_root / "cache"
-        return None
-
     @model_validator(mode="after")
     def detect_euid_to_use(self) -> Self:
         if self.bin_dir is None and self.install_root is not None:
             self.bin_dir = self.install_root / "bin"
+        if self.cache_dir is None and self.install_root is not None:
+            self.cache_dir = self.install_root / "cache"
         return self
 
     def setup_PATH(self, no_cache: bool = False) -> None:
