Skip to content

Fix root-as-dropped-euid exec env and restore Binary.load_or_install#32

Merged
pirate merged 4 commits intomainfrom
claude/magical-pascal-iR9iO
Apr 21, 2026
Merged

Fix root-as-dropped-euid exec env and restore Binary.load_or_install#32
pirate merged 4 commits intomainfrom
claude/magical-pascal-iR9iO

Conversation

@pirate
Copy link
Copy Markdown
Member

@pirate pirate commented Apr 21, 2026

Summary

Fixes three paper cuts that show up when running abxpkg as root on Linux (the common CI/container case) plus one README typo:

  1. Binary.load_or_install restored. This convenience helper got dropped earlier but every downstream plugin hook in abx-plugins still calls it (npm/pip/puppeteer/brew/apt/bash/cargo/chromewebstore/…). Its absence made every hook crash with the opaque message "npm install failed: load_or_install". The restored method follows the documented semantics: try load() first, fall back to install() when the binary isn't already valid.

  2. BinProvider.exec env when dropping from root. When the caller is root and the provider's EUID points at a non-root owner (e.g. brew owned by brewuser / linuxbrew), preexec_fn=drop_privileges correctly switches the subprocess's UID but the environment was still being built from fallback_env (root's HOME/LOGNAME/USER). Brew then blew up with Permission denied @ dir_s_mkdir - /root/.cache/Homebrew because the dropped subprocess couldn't write into root's home. The fix picks sudo_env (target user's identity) when we're going to drop privileges.

  3. BrewProvider shim dir traversability. The _refresh_bin_link helper creates <install_root>/bin/<name> in the parent Python process (running as root in the above flow). The later self.exec drops to brew's owner UID for version probing, and that dropped subprocess couldn't traverse the root-owned ancestor directories. Now we os.chown the shim dir and bump the other/group exec bits on ancestors when we're about to drop privileges, so the version probe reaches the symlink.

  4. README typo. The CargoProvider section said set \install_root=Path(...)` or `install_root=Path(...)`— the second reference is the alias, so it now readscargo_root=Path(...)`.

Test plan

  • uv run prek run --all-files — clean
  • uv run pytest tests/test_brewprovider.py (as root on Linux, /home/linuxbrew/.linuxbrew owned by a non-root user) — all 7 tests pass with the exec env + shim chmod fix
  • Downstream abx-plugins hooks that call binary.load_or_install() now complete instead of failing fast with "load_or_install".
  • CI matrix

Open in Devin Review

Summary by cubic

Fixes root-to-user exec env and brew shim traversability when running as root to prevent Homebrew permission errors; adds a PuppeteerProvider cache-dir override. Note: Binary.load_or_install remains removed — update hooks to call install().

  • New Features

    • PuppeteerProvider: add browser_cache_dir to override computed cache_dir for PUPPETEER_CACHE_DIR.
  • Bug Fixes

    • BinProvider.exec: when root drops to provider EUID, use sudo_env (target user's HOME/USER/LOGNAME).
    • BrewProvider: chown shim dir to brew owner and add execute bits on ancestor dirs so dropped subprocess can reach the symlink.
    • Docs: fix cargo_root alias typo in the CargoProvider README.

Written for commit 501cc33. Summary will update on new commits.

- Binary.load_or_install: restore the "try load(), fall back to install()"
  helper that abx-plugins hooks rely on after it got inadvertently dropped.
- BinProvider.exec: when running as root and dropping privileges to a
  non-root target UID, use sudo_env (target user's HOME/LOGNAME/USER)
  instead of fallback_env so the dropped subprocess finds its own cache/
  config dirs (fixes brew "Permission denied ~/.cache/Homebrew" under root).
- BrewProvider._refresh_bin_link: when running as root but about to drop
  to brew's owner UID, chmod the managed shim dir's ancestors to be
  traversable by that UID so version probes from the dropped-privilege
  subprocess can reach the symlink.
- README: fix cargo_root alias typo in the CargoProvider section.
devin-ai-integration[bot]

This comment was marked as resolved.

Devin Review caught that ``if loaded.is_valid and not no_cache:`` applied the
caller's ``no_cache`` flag to the value we just got back from ``load()``.
``load()`` already honours the flag internally, so a valid result is fresh
regardless — the ``and not no_cache`` guard was forcing a redundant
``install()`` whenever ``no_cache=True`` even though the binary already
resolved. Drop the redundant condition.
cubic-dev-ai[bot]

This comment was marked as resolved.

The computed ``cache_dir = install_root/cache`` default works for the
managed lib-dir layout but forced downstream plugins to treat
``PUPPETEER_CACHE_DIR`` as ``install_root.parent``, which broke callers
that pin the cache at an arbitrary path (e.g. ``lib/puppeteer/chrome``).

Add a writable ``browser_cache_dir`` field that wins over the computed
default so hooks can pass the user-configured PUPPETEER_CACHE_DIR through
to the provider verbatim while still using ``install_root`` / ``bin_dir``
for metadata and shim locations.
Per maintainer feedback: the removal was intentional — callsites should
use ``install()`` directly (which is already a no-op short-circuit when
``is_valid`` and no ``no_cache`` is set). Reverting the restored helper
and leaving it to the plugin hooks to adopt ``install()``.
@pirate pirate merged commit 0f297d8 into main Apr 21, 2026
94 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