Skip to content

Honor PUPPETEER_CACHE_DIR + PLAYWRIGHT_BROWSERS_PATH env vars#33

Merged
pirate merged 33 commits intomainfrom
claude/magical-pascal-iR9iO
Apr 23, 2026
Merged

Honor PUPPETEER_CACHE_DIR + PLAYWRIGHT_BROWSERS_PATH env vars#33
pirate merged 33 commits intomainfrom
claude/magical-pascal-iR9iO

Conversation

@pirate
Copy link
Copy Markdown
Member

@pirate pirate commented Apr 21, 2026

Summary

Puppeteer and Playwright both expose standard env vars that users (and downstream CI) expect to control browser cache locations. Previously the provider field-default factories only picked up abxpkg-specific aliases, silently clobbering the user's env when constructing the provider.

  • PuppeteerProvider.browser_cache_dir now hydrates from PUPPETEER_CACHE_DIR when unset; the existing install_root/cache fallback still kicks in when neither is set.
  • PlaywrightProvider.install_root now hydrates from PLAYWRIGHT_BROWSERS_PATH when ABXPKG_PLAYWRIGHT_ROOT / ABXPKG_LIB_DIR/playwright are also unset, so a user-exported browsers path propagates into the provider instead of being overwritten.

Both providers already emit the matching env var in ENV, so this just removes the asymmetry between "abxpkg exports these" and "abxpkg reads these".

Test plan

  • uv run prek run --all-files — clean
  • CI matrix

Open in Devin Review

Summary by cubic

Make browser cache management opt-in for puppeteer and playwright. We now export PUPPETEER_CACHE_DIR / PLAYWRIGHT_BROWSERS_PATH to <install_root>/cache only in managed mode; unmanaged mode passes ambient defaults through. Resolution asks upstream tools first; bin_dir shims are output-only. Sandbox proxy handling hardened, including through sudo.

  • Refactors

    • playwright: export PLAYWRIGHT_BROWSERS_PATH only when install_root is set (to <install_root>/cache); resolve via playwright-core first and scope hits to the managed cache; uninstall() rmtrees the real browser dir then removes the shim; the env KEY=VAL -- ... sudo wrapper forwards the cache dir and NO_PROXY/no_proxy; define a sandbox NO_PROXY allowlist locally.
    • puppeteer: export PUPPETEER_CACHE_DIR only when install_root is set; unmanaged mode is pure passthrough; resolve via the CLI first; uninstall() rmtrees the real browser dir then removes the shim; INSTALLER_BINARY bootstraps from <install_root>/npm/node_modules/.bin and is cached; resolve alias install args and fall back to newest mtime when versions aren’t SemVer.
    • brew: search Cellar/opt/PATH first; refresh the managed shim only after resolving the true target.
    • npm/pnpm/goget/brew: make bin_dir shim refresh idempotent; skip rewrite when it already points at the target to avoid inode/mtime churn.
    • CI: use runner group Default for Linux and discovery jobs; matrix includes os_name; live-test discovery emits os and os_name; deploy-pages and release jobs also use the group; precheck installs Node 22. Tests handle macOS shell-script shims by parsing their exec target.
    • Both providers: drop the cache_dir field and inline <install_root>/cache; only pass --path= and create the cache dir in managed mode. README updated.
  • Bug Fixes

    • AptProvider/BrewProvider: setup_PATH now rebuilds when PATH is empty.
    • brew: hoist linked_bin resolution so the fallback path works after the shim-first cleanup.
    • puppeteer/playwright: fix uninstall() ordering to avoid dangling shims by deleting the real browser dir before unlinking the shim.
    • puppeteer: guard default_abspath_handler when bin_dir is None to avoid ambient-mode crashes; _resolve_installed_browser_path uses provider install_args to resolve aliases and falls back to newest mtime when versions aren’t SemVer; uninstall() now forwards install_args to the browser-path resolver so alias installs are removed correctly.
    • puppeteer/playwright: auto-apply a sandbox-safe NO_PROXY default when ambient values are unset or include .googleapis.com/.google.com; playwright also forwards these through the sudo env wrapper.
    • puppeteer/playwright: make bin_dir shim refresh idempotent; skip rewrite when it already points to the target to avoid mtime bumps on macOS .app shims.

Written for commit 4aa46ab. Summary will update on new commits.

Puppeteer and Playwright both expose standard env vars that users (and
downstream CI) expect to control browser cache locations. Previously
``browser_cache_dir`` / ``install_root`` on those providers only came
from explicit abxpkg aliases, silently clobbering the user's env.

- ``PuppeteerProvider.browser_cache_dir`` now hydrates from
  ``PUPPETEER_CACHE_DIR`` when unset; the existing
  ``install_root/cache`` fallback still kicks in when neither is set.
- ``PlaywrightProvider.install_root`` now hydrates from
  ``PLAYWRIGHT_BROWSERS_PATH`` when ``ABXPKG_PLAYWRIGHT_ROOT`` /
  ``ABXPKG_LIB_DIR/playwright`` are also unset, so a user-exported
  browsers path propagates into the provider instead of being
  overwritten.

Both providers already emit the matching env var in ``ENV``, so this
just removes the asymmetry between "abxpkg exports these" and "abxpkg
reads these".
devin-ai-integration[bot]

This comment was marked as resolved.

cubic-dev-ai[bot]

This comment was marked as resolved.

claude added 3 commits April 21, 2026 21:46
When a provider instance is constructed (or copied via
``get_provider_with_overrides``) and its ``_INSTALLER_BINARY`` gets
resolved out-of-band — e.g. a plugin hook does a ``provider.INSTALLER_BINARY()``
availability check before handing the provider to ``Binary.install()``
— the subsequent ``install`` -> ``setup_PATH`` call hit the
``_INSTALLER_BINARY is None`` short-circuit and returned without
populating ``self.PATH``. The post-install ``load()`` then searched an
empty PATH, failed to find the freshly-installed binary, and raised
``Installed package did not produce runnable binary ...`` (rolling back
a perfectly good install).

Add ``not self.PATH`` to the rebuild condition on both AptProvider and
BrewProvider so an empty PATH always forces a fresh PATH discovery
regardless of whether the installer binary is already cached.
…edence

Before this commit ``install_root`` did double duty as both the abxpkg
managed root and the ``PLAYWRIGHT_BROWSERS_PATH`` export, so callers had
no way to pin the browsers-path separately and explicit ``install_root``
kwargs always clobbered a user-set ``PLAYWRIGHT_BROWSERS_PATH`` env var.

Split the two concepts:

- ``install_root`` keeps its role as the managed provider root (bin_dir
  symlinks, nested npm prefix, derived.env cache).
- A new ``browsers_path`` field hydrates from ``PLAYWRIGHT_BROWSERS_PATH``
  and always wins when set.
- ``resolved_browsers_path`` computes the final path with the same
  precedence puppeteer uses:
    1. explicit ``browsers_path`` (field or env) wins
    2. else ``install_root/cache`` when a managed root is pinned
    3. else ``None`` (let playwright pick its OS default)
- ``ENV`` + the sudo ``env KEY=VAL`` wrapper both emit the resolved
  path instead of ``install_root`` directly, so the subprocess and the
  host stay consistent.

Matches the documented semantics for PUPPETEER_CACHE_DIR: the more
specific cache-dir env var always beats the less specific install-root
option; abx-plugins never has to set the env var itself because
``ABXPKG_LIB_DIR`` threads through ``install_root`` and the default
``install_root/cache`` layout covers the managed case.
…dence

Both provider sections now spell out the three-tier precedence used by
the newly-exposed override fields:
  1. explicit ``browser_cache_dir`` / ``browsers_path`` (or their matching
     env vars ``PUPPETEER_CACHE_DIR`` / ``PLAYWRIGHT_BROWSERS_PATH``) wins
  2. else ``<install_root>/cache`` when an install root is pinned
  3. else ``None`` (let the CLI pick its OS default)

Also pulled the "downloads live under install_root/cache" factoid out of
PuppeteerProvider's install-root bullet and into the dedicated
cache-dir bullet where it belongs.
devin-ai-integration[bot]

This comment was marked as resolved.

claude added 2 commits April 21, 2026 22:18
…Puppeteer

Match the Puppeteer provider's field names so both browser-installing
providers expose the same API surface:

- writable field: ``browser_cache_dir`` (was ``browsers_path``)
- computed property: ``cache_dir``   (was ``resolved_browsers_path``)

Both still default-factory from their respective env vars
(``PUPPETEER_CACHE_DIR`` / ``PLAYWRIGHT_BROWSERS_PATH``) and use the
same three-tier precedence documented in the README.
…che_dir

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.
devin-ai-integration[bot]

This comment was marked as resolved.

cubic-dev-ai[bot]

This comment was marked as resolved.

…l_root

Per maintainer feedback: ``cache_dir`` was always 1:1 with
``install_root``, so the writable field was redundant. Collapse it
to a single ``@computed_field`` on both providers:

    cache_dir = <install_root>/cache  when install_root is set
    cache_dir = None                  when install_root is unset

When ``install_root`` is unset the provider exports neither
``PUPPETEER_CACHE_DIR`` nor ``PLAYWRIGHT_BROWSERS_PATH`` to
subprocesses, so ``puppeteer-browsers`` / ``playwright`` fall back to
whatever the ambient env + their own OS default dictates. When
``install_root`` is pinned, the computed ``cache_dir`` flows through
to ``ENV``, the sudo ``env KEY=VAL`` wrapper, ``--path=...``
install args, ``default_uninstall_handler``, and
``default_abspath_handler`` scope checks — all call sites already
guarded on ``is not None`` so removing the field changes no behaviour
when install_root is set.

Provider field surface is now just ``install_root`` + ``bin_dir``
(plus the class-level ``INSTALLER_BIN`` constant), matching the
maintainer's "only install_root, cache_dir, bin_dir; no duplicates"
direction.
cubic-dev-ai[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

claude added 4 commits April 21, 2026 22:40
…root=None

With ``install_root=None`` the computed ``cache_dir`` used to return
``None`` unconditionally, so ``uninstall()`` and the ``abspath``
scope-check couldn't target a user-configured
``PUPPETEER_CACHE_DIR`` / ``PLAYWRIGHT_BROWSERS_PATH`` — ``load()``
still worked because the subprocess inherited the ambient env, but
``uninstall()`` silently skipped cleanup of the user's custom
directory.

Fix the computed property for both providers so ``cache_dir`` resolves
the ambient env var whenever ``install_root`` is unset:

    install_root set                   -> <install_root>/cache
    install_root=None + env var set    -> Path($PUPPETEER_CACHE_DIR)
                                          / Path($PLAYWRIGHT_BROWSERS_PATH)
    install_root=None + env unset      -> None  (CLI's OS default)

Every downstream call site (install ``--path=`` arg, ENV export, sudo
``env KEY=VAL --`` wrapper, ``default_uninstall_handler``,
``default_abspath_handler`` scope checks, cache mkdir/chown) already
routes through ``self.cache_dir`` with ``is not None`` guards, so
``load()``/``install()``/``uninstall()`` now all target the same
directory the user configured externally without any other changes.
… when install_root=None

``cache_dir`` is now a tiny computed helper that returns
``<install_root>/cache`` when ``install_root`` is pinned and ``None``
otherwise — no env-var fallback. This flips the ``install_root=None``
mode to pure passthrough:

- The caller's ambient ``PUPPETEER_CACHE_DIR`` /
  ``PLAYWRIGHT_BROWSERS_PATH`` passes through to every subprocess
  unchanged (abxpkg exports nothing of its own).
- ``load()`` trusts whatever ``puppeteer-browsers list`` /
  playwright-core ``executablePath()`` reports — no scoping applied.
- ``uninstall()`` is a no-op when ``install_root=None``: we refuse to
  rmtree the user's cache because we don't own it. Users who want
  managed uninstall should set ``install_root``; users who stay in
  ambient mode manage their own cache via the CLI or their own tooling.
- ``install()`` only passes ``--path=`` / sets
  ``PLAYWRIGHT_BROWSERS_PATH`` when we own the root; otherwise the
  CLIs install into their own default.

Every existing call site already guarded on ``self.cache_dir is not
None``, so dropping the env-var branch changes no managed-mode
behaviour — it just stops abxpkg from claiming ownership of a cache
dir the user controls.
``puppeteer-browsers clear`` wipes the whole cache (no per-browser
filter) and ``playwright uninstall`` rejects browser-name arguments,
so neither CLI can do the per-browser uninstall abxpkg exposes.

Switch both uninstall handlers to:

1. ``self.load(bin_name, no_cache=True)`` — playwright-core's
   ``executablePath()`` / puppeteer-browsers' ``list`` already read
   the right path from the subprocess env, so load() handles all
   three cases uniformly:
     - managed ``install_root`` → abxpkg exports the env var itself
     - unmanaged default       → CLI's own OS default
     - custom ``PUPPETEER_CACHE_DIR`` / ``PLAYWRIGHT_BROWSERS_PATH`` →
       ambient env passes through via ``build_exec_env``
2. Walk up from the resolved abspath to find the browser's top-level
   dir (``<cache>/<browser_name>/`` for puppeteer,
   ``<cache>/<browser_name>-<buildId>/`` for playwright) and rmtree
   it.

No more ``self.cache_dir`` lookups in either uninstall handler, and
the existing ``bin_dir`` symlink cleanup still runs first so ``load``
stops resolving a stale shim if rmtree fails partway through.
``cache_dir`` was a one-line computed helper (``install_root/cache``
when managed, ``None`` otherwise) that only existed as a convenience
alias — every callsite already branched on ``is not None``. Remove
the computed field and inline ``install_root / "cache"`` at each use:

- ``ENV`` now returns ``{}`` when ``install_root`` is None and
  ``{"PUPPETEER_CACHE_DIR": "<install_root>/cache"}`` /
  ``{"PLAYWRIGHT_BROWSERS_PATH": "<install_root>/cache"}`` otherwise.
- ``setup`` creates ``<install_root>/cache`` next to the existing
  ``install_root.mkdir``.
- ``_normalize_install_args`` / ``_list_installed_browsers`` pass
  ``--path=<install_root>/cache`` only when managed.
- ``_cleanup_partial_browser_cache`` takes an early-return when
  ``install_root`` is None and uses a local ``cache_dir`` var for the
  glob/rmtree loop.
- Playwright ``build_exec_env`` sudo wrapper and
  ``default_abspath_handler`` scope check both use a local
  ``cache_dir`` var under ``install_root is not None``.
- Uninstall handlers already resolved the browser dir via ``load()``
  in the previous commit, so they don't touch cache_dir at all.

Net result: both providers' public field surface is just
``install_root`` + ``bin_dir``, matching the "only
install_root/bin_dir" direction.
cubic-dev-ai[bot]

This comment was marked as resolved.

cubic-dev-ai[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

Core principle (per maintainer): the symlinks/shims abxpkg writes under
``bin_dir`` are a human-convenience side-effect of install, not a
source of truth. ``load()`` and every other internal lookup must
always consult the underlying package manager / CLI instead.

Audit result — only three providers had a "shim-first" short-circuit
in ``default_abspath_handler``; the others (``docker``, ``bash``
wrappers; ``npm``/``pip``/``pnpm``/``yarn``/``uv``/``cargo``/``gem``/
``goget`` where bin_dir IS the package manager's install location, not
a shim of something upstream) were already correct.

Fixed:
- ``BrewProvider``: removed the initial ``bin_abspath(bin_name,
  PATH=str(self.bin_dir))`` short-circuit. Now always computes the
  brew Cellar / opt / PATH search list first and only refreshes the
  managed shim to match the freshly-resolved target.
- ``PuppeteerProvider``: removed the ``bin_dir/<name>`` shim check
  before falling back to ``puppeteer-browsers list``. Now always asks
  the CLI first, then refreshes the shim to point at the fresh path.
- ``PlaywrightProvider``: same — always asks ``playwright-core``'s
  ``executablePath()`` first, shim becomes output-only.

Also updated the uninstall-handler docstrings for both puppeteer and
playwright to reflect that dropping the shim is a cosmetic cleanup
(to keep managed PATH tidy) rather than a correctness requirement for
``load()``.
@cubic-dev-ai
Copy link
Copy Markdown
Contributor

cubic-dev-ai Bot commented Apr 21, 2026

You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment @cubic-dev-ai review.

claude added 2 commits April 21, 2026 23:23
- ``BrewProvider.default_abspath_handler``: ``linked_bin`` needs to be
  resolved at the top of the function since the fallback
  (``brew list --formula ...``) branch at line ~438 still references
  it. Hoist it back above the cellar/opt search rather than inside
  the ``if abspath:`` block.
- ``tests/test_puppeteerprovider.py``: replace ``provider.cache_dir``
  with the inline ``provider.install_root / "cache"`` now that the
  field has been removed.
…oad()

Pirate review caught the ordering bug I introduced: ``default_uninstall_handler``
unlinked the convenience shim first, then called ``load(bin_name)`` to find
the real browser directory. But ``default_abspath_handler`` refreshes the
managed shim to point at the freshly-resolved target, so by the time
rmtree ran, the shim had been re-created pointing at the browser dir
we were about to delete — leaving a dangling symlink afterwards.

Swap the order: resolve the browser directory via the CLI directly
(``_resolve_installed_browser_path`` for puppeteer,
``_playwright_browser_path`` for playwright), rmtree the browser dir,
THEN drop the shim last. The internal resolvers don't touch bin_dir,
so the shim only goes away when we explicitly unlink it.

README: replace the stale Playwright note that claimed we "leave
playwright's OS-default cache untouched when install_root is unset"
with the actual current behaviour — uninstall rmtrees the resolved
browser directory in both managed and passthrough modes, since the
CLI's own uninstall has no per-browser argument.
devin-ai-integration[bot]

This comment was marked as resolved.

claude added 4 commits April 21, 2026 23:43
…t/cache

Devin caught the test drift from the PLAYWRIGHT_BROWSERS_PATH rework:
browsers now land in ``<install_root>/cache/chromium-<buildId>/``
(matching the ``install_root/cache`` default abxpkg pins), not
directly under ``install_root``. Update every place that iterates
``playwright_root`` or ``install_root`` looking for ``chromium-*/``
dirs to walk ``<root>/cache`` instead — covering the chromium install
lifecycle assertions, the ``--no-shell`` carveout check, the update
test's resolved-target ancestry check, and the dry-run "no browsers
got downloaded" check.

Also points the symlink ancestry assertion in
``test_chromium_install_puts_real_browser_into_managed_bin_dir`` at
``playwright_root/cache`` since that's the actual
``PLAYWRIGHT_BROWSERS_PATH`` root the symlinks resolve into.
Devin caught the parity bug I introduced: ``_refresh_symlink`` asserts
``bin_dir is not None``, but in global/unmanaged mode
(``install_root=None``) ``bin_dir`` is ``None`` and the ``except OSError``
doesn't catch the resulting ``AssertionError`` — crashing
``default_abspath_handler`` instead of returning the resolved path.

Add the same early-return guard ``PlaywrightProvider`` already has:
when ``bin_dir`` is ``None`` we return ``resolved`` directly without
attempting a shim refresh.
…t routes Google domains direct

The ``CLAUDE_SANDBOX_NO_PROXY`` constant has been sitting in the
PuppeteerProvider as a post-failure hint only; in sandboxed envs
(Claude, some CI runners) the ambient ``NO_PROXY`` includes
``.googleapis.com`` / ``.google.com``, which tells both
``@puppeteer/browsers`` and ``playwright install`` to bypass the
egress proxy when fetching from ``storage.googleapis.com`` /
``cdn.playwright.dev`` — and the direct connection then fails DNS or
returns 503 because sandbox egress only works through the proxy.

Wire the same sandbox-safe allowlist into both providers' ``ENV``:

- If ``NO_PROXY`` / ``no_proxy`` is unset or contains
  ``.googleapis.com`` / ``.google.com``, replace it in the subprocess
  env with ``CLAUDE_SANDBOX_NO_PROXY`` (localhost / metadata-service /
  cluster-local only) so the browser download goes through the proxy.
- If the caller has a non-sandbox-problematic ``NO_PROXY`` (e.g.
  ``localhost,internal.corp``), pass it through unchanged — we only
  override the two known-bad patterns.

Playwright imports the constant from PuppeteerProvider to avoid a
second copy; both providers share the exact same allowlist so tests
can assert one value.
…ed install_args

When the shim-first short-circuit was removed from
``default_abspath_handler``, ``load()`` started going through
``_resolve_installed_browser_path`` for every query. That helper
computed the canonical browser name from ``bin_name + install_args``,
but ``default_abspath_handler`` didn't forward any ``install_args``,
so it fell back to ``[bin_name]`` — which meant ``load("chrome")``
looked for ``puppeteer-browsers list`` entries whose browser name
was ``chrome``, missing installs that landed as ``chromium@latest``
via a ``{"chrome": {"install_args": ["chromium@latest"]}}`` override
(exactly the scenario ``test_chrome_alias_installs_real_browser_binary``
exercises).

Fix: when ``install_args`` isn't passed explicitly, fall back to
``self.get_install_args(bin_name)`` so the provider's own
handler-overrides map the alias (``chrome`` → ``chromium@latest``)
before ``_browser_name`` extracts the canonical package name.
claude added 2 commits April 22, 2026 00:28
…when no SemVer

Chromium build IDs (e.g. ``1618494``) aren't valid SemVer, so
``SemVer.parse`` returns ``None`` for all candidates. When two
builds end up in the cache (typical ``chromium@latest`` reinstall
flow: original build + newly-resolved ``latest``) we fell through
to ``return None`` because ``parsed_candidates=[]`` and
``len(candidates) > 1``. ``provider.install('chrome', no_cache=True)``
then failed the post-install load with "Installed package did not
produce runnable binary 'chrome'" — exactly the CI failure on
``test_chrome_alias_installs_real_browser_binary``.

Add a deterministic fallback: sort unparseable candidates by file
mtime and pick the newest, so a ``latest`` reinstall still resolves
to the freshly-downloaded build instead of bailing out.
…les/.bin

Fresh provider copies built by Binary.load() via
get_provider_with_overrides start with _INSTALLER_BINARY=None, so
super().INSTALLER_BINARY() falls through to searching the env/brew
installer providers — none of which know about our hermetic
<install_root>/npm/node_modules/.bin layout. The install itself writes
puppeteer-browsers there, but subsequent load()s couldn't find it,
making _list_installed_browsers() silently return empty and
default_abspath_handler return None → Binary.load() raises
`BinaryLoadError: ERRORS={}`.

Mirror PlaywrightProvider's local_cli fallback: check the hermetic
npm bin first, then wrap the result in an EnvProvider.load() and
persist it via write_cached_binary so repeat lookups are cheap.

https://claude.ai/code/session_01Xt1YPCMzQrrFihDgVEYy4S
devin-ai-integration[bot]

This comment was marked as resolved.

pirate and others added 3 commits April 21, 2026 20:00
…a sudo env wrapper

Two Devin review findings:

1. binprovider_playwright.py imported CLAUDE_SANDBOX_NO_PROXY from
   binprovider_puppeteer.py, violating the "each binprovider owns its
   own provider-specific logic" rule in AGENTS.md. Define the constant
   locally instead.

2. exec() already wraps commands with /usr/bin/env KEY=VAL so that
   PLAYWRIGHT_BROWSERS_PATH survives sudo's env_reset, but the
   NO_PROXY / no_proxy values set by ENV were not being forwarded via
   the same mechanism -- sudo stripped them before reaching playwright,
   defeating the sandbox NO_PROXY fix on Linux (where euid=0 routes
   every subprocess through sudo -n). Forward NO_PROXY/no_proxy as
   CLI-arg assignments alongside PLAYWRIGHT_BROWSERS_PATH.
devin-ai-integration[bot]

This comment was marked as resolved.

claude added 3 commits April 22, 2026 16:27
…atches

On macOS, chrome/chromium ship as ``.app`` bundles so the managed
``bin_dir`` shim is a shell-script launcher (``#!/bin/sh\nexec <path>``),
not a symlink. ``default_abspath_handler`` calls ``_refresh_symlink`` on
every ``load()``, which unconditionally deleted + rewrote the shim --
bumping its mtime each time. Tests that call ``install()`` then
``assert_shallow_binary_loaded`` (which re-stats the shim via
``get_abspath(no_cache=True)``) then observed ``loaded_mtime !=
resolved.stat().st_mtime_ns``, failing across every macOS puppeteer/
playwright test (8 playwright tests + test_chrome_alias in puppeteer).

Linux uses a real symlink and ``.resolve().stat()`` follows it to the
browser binary, whose mtime is stable -- so Linux CI was already green.

Skip the unlink+rewrite when the existing shim already points at
``target`` (symlink target equals path, or shell-script contents match
what we'd write). ``install()`` still always resolves the fresh path,
and a stale shim pointing at an old target still gets replaced.
Brew / Npm / Pnpm / Goget providers all refreshed their managed
``bin_dir`` shim on every ``default_abspath_handler`` call (i.e. every
``load()``) by unconditionally ``unlink`` + ``symlink_to``. Even for
symlinks -- where ``.resolve().stat()`` follows the link and returns
the target's stable mtime -- this still churns the shim's inode on
every load, which invalidates fingerprint caches keyed on inode and
causes unnecessary filesystem thrash in tight lifecycle loops.

Skip the unlink + rewrite when the existing symlink already resolves
to ``target``. Matches the idempotency check already present in the
base class ``_link_loaded_binary`` and the same fix just applied to
Puppeteer/Playwright ``_refresh_symlink``.
devin-ai-integration[bot]

This comment was marked as resolved.

claude added 7 commits April 22, 2026 16:50
…olver

``default_uninstall_handler`` derived ``browser_name`` from the caller's
``install_args`` but then called ``_resolve_installed_browser_path``
without forwarding them. The resolver re-runs ``get_install_args``
internally and, when the caller passed an alias that differs from the
provider's registered overrides (e.g. ``chrome`` → ``chromium@latest``),
picks a different ``browser_name`` than the one we just derived. The
subsequent ``parent.name == browser_name`` match against the resolved
path then fails silently, skipping the rmtree and leaking the browser
cache to disk.

Forward the already-resolved ``install_args`` so both derivations stay
in sync.
…acOS

Two playwright tests assert ``loaded_abspath.resolve()`` lands inside
``playwright_root/cache``. On Linux the shim is a plain symlink so
``.resolve()`` follows naturally. On macOS chromium ships inside a
``.app`` bundle and the shim is a shell script (``exec '<path>'``)
instead -- ``.resolve()`` on a regular file just returns the file
itself, failing the ``in updated_target.parents`` assertion.

Add ``_resolve_shim_target()`` that falls back to parsing the ``exec``
line from the shell script when ``.resolve()`` doesn't follow. Also
update ``copy_seeded_playwright_root()`` to rewrite the shell script's
hardcoded absolute path from the seeded root to the per-test copy
(previously it only repointed symlinks), so the pre-``load()`` shim
state is consistent across both shim flavors.
…x CI to self-hosted

1. ``tests/test_playwrightprovider.py::_resolve_shim_target``: on macOS
   ``$TMPDIR`` lives under ``/var/folders/...`` → ``/private/var/...``,
   so ``shim.resolve()`` always differs from ``shim`` even for a plain
   regular file. The previous ``resolved != shim`` short-circuit
   skipped the shell-script parsing and returned the shim path itself.
   Key the branch off ``is_symlink()`` instead so the shell-script
   case (macOS ``.app`` bundle, where a plain symlink would break
   dyld's ``@executable_path``-relative Framework loading) actually
   parses the ``exec '<path>'`` line.

2. Switch Linux CI jobs (precheck, discover-*, build matrix Ubuntu
   entries, live-integration Linux entries, deploy-pages, release) from
   ``ubuntu-latest`` to the org's new self-hosted Linux runner. macOS
   matrix entries keep ``macOS-latest`` since the self-hosted runner is
   x64 Linux only.
…ted or self-hosted runners

Switch from the literal ``self-hosted`` label (which would only pick
our self-hosted Linux runner) to ``runs-on: {group: Default}``. The
Default runner group contains both GitHub-hosted runners and our new
self-hosted x64 Linux runner, so the scheduler picks whichever is
available first.

- precheck / discover-standard-tests / discover-live-tests /
  deploy-pages / release: simple ``runs-on: {group: Default}``.
- build matrix: Linux entries use ``os: {group: Default}`` (mapping
  values in matrix), macOS keeps the ``macOS-latest`` string. Added an
  ``os_name`` field for the job name since a mapping doesn't render
  nicely. ``runs-on: \${{ matrix.target.os }}`` evaluates to either
  the mapping or the string; GitHub Actions accepts both.
- live-integration: discover script now emits ``os`` as either
  ``{"group":"Default"}`` or ``"macOS-latest"`` in the matrix JSON,
  and includes an ``os_name`` for display.
``uv run prek run --all-files`` runs the pyright hook, which executes
pyright (a bundled Node.js app). On GH-hosted ``ubuntu-latest`` Node
22 is pre-installed, but on the self-hosted runner system Node is too
old: pyright's ``vendor.js`` uses class-field syntax and throws
``SyntaxError: Unexpected token =`` against a Node ~10-era
``cjs/loader.js``. Pin a modern Node via ``actions/setup-node@v6`` in
the precheck job so the hook has a runtime it can actually parse.
The self-hosted Linux runner's work dir lives on /runner, which is a
separate filesystem from /root. uv's default cache lives under
$HOME/.cache/uv (or $XDG_CACHE_HOME/uv if set) and uv hard-links from
the cache into the venv — hard-links can't cross filesystem
boundaries, so uv silently falls back to copying every dep, adding
tens of seconds to every ``uv sync`` / ``uv venv``.

Pin XDG_CACHE_HOME to a path on /runner (same fs as the work dir) so
uv keeps its cache there and hard-linking succeeds. Wrapped in
``if: runner.environment == 'self-hosted'`` so GitHub-hosted runs
(which don't have /runner and run as unprivileged ``runner`` user)
aren't affected.

Added to every job that runs uv: precheck, discover-standard-tests,
build, discover-live-tests, live-integration, deploy-pages build,
release-state.
@pirate pirate merged commit 665d78a into main Apr 23, 2026
9 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants