Align plugin hooks with current abxpkg API and fix dangling-symlink chmod#26
Align plugin hooks with current abxpkg API and fix dangling-symlink chmod#26
Conversation
…hmod - Replace the removed ``provider.INSTALLER_BIN_ABSPATH`` attribute access in apt/brew/cargo/npm/pip hook scripts with a ``try: provider.INSTALLER_BINARY(); except Exception: ...`` gate so hooks still skip cleanly when the installer binary is missing. - Replace removed PuppeteerProvider ``browser_cache_dir`` / ``browser_bin_dir`` and ChromeWebstoreProvider ``extensions_dir`` / NpmProvider ``npm_prefix`` / PipProvider ``pip_venv`` kwargs with the current ``install_root`` / ``bin_dir`` fields so hooks pass pyright without relying on validation-alias-only names. - enforce_lib_permissions: use ``lstat()`` and skip symlinks so a dangling symlink in ``lib/env/bin/`` doesn't crash the post-install lockdown pass. - pyproject: add ``abx-plugins = "1 second"`` to ``exclude-newer-package`` so hook scripts can resolve a fresh self-ref without the 14-day cutoff pinning to an old ``abx-pkg``-named dependency.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…e_dir Devin Review correctly flagged that deriving ``install_root = PUPPETEER_CACHE_DIR.parent`` silently broke callers that pin the cache at an arbitrary path like ``lib/puppeteer/chrome`` — the provider would then resolve ``cache_dir = install_root/cache``, which mismatches the configured path (and fails the screenshot e2e test that explicitly asserts ``PUPPETEER_CACHE_DIR == lib_dir/puppeteer/chrome``). Use the new ``browser_cache_dir`` override on ``PuppeteerProvider`` so the user-configured cache dir flows through verbatim, while ``install_root`` / ``bin_dir`` still give the provider a sibling root for npm helper / derived.env metadata.
CI status notesMost per-plugin failures on this run trace to a single upstream gap: this repo's CI clones Once ArchiveBox/abxpkg#32 merges, a re-run of this workflow should clear the cascade. The standalone changes here (plugin hook API alignment, Addressing the Devin review: the fragile Generated by Claude Code |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Pyright in the abx-plugins CI clones ``abxpkg`` from ``main`` (which
doesn't yet carry the ``browser_cache_dir`` field being added in the
paired abxpkg PR), so calling ``PuppeteerProvider(browser_cache_dir=...)``
directly tripped ``reportCallIssue``. Dispatching through
``PuppeteerProvider.model_validate({...})`` keeps the literal
``PUPPETEER_CACHE_DIR`` passthrough behaviour at runtime while going
through pydantic's dict-based validation path that pyright doesn't
param-check.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Devin Review correctly flagged that the dangling-symlink chmod protection applied to the ``lib/`` tree walk wasn't also applied to the sibling walk over non-``lib`` config entries. The two loops share the same ``fp.chmod`` pattern, and a dangling symlink under e.g. ``~/.config/abx/personas/…`` would crash with the same ``FileNotFoundError`` the first loop already guards against. Mirror the ``lstat()`` + ``S_ISLNK`` skip in both loops, and apply the same check to the top-level ``entry`` file branch.
|
NO DO NOT RESTORE load_or_install, fix all the callsites to simply use install(). stop reverting intentional changes and re-introducing deprecated behavior. |
Per maintainer feedback on PR #26: restore the intentional removal of ``Binary.load_or_install`` upstream by moving the hook scripts and plugin tests to call ``install()`` directly. ``install()`` already short-circuits when the binary is already valid, so functional behaviour is preserved.
|
review the https://github.com/ArchiveBox/abxpkg/releases release notes using the |
The hook now calls ``binary.install()`` directly (instead of the removed ``load_or_install``), so the ``BinaryInstallError`` bubbling up says "Unable to install binary ..." not "Unable to load or install ...". Match the new action string in the stderr assertion.
There was a problem hiding this comment.
1 issue found across 17 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="abx_plugins/plugins/bash/tests/test_bash_provider.py">
<violation number="1" location="abx_plugins/plugins/bash/tests/test_bash_provider.py:131">
P3: Fix the expected stderr text to match the hook's actual failure message.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| # Should fail since binary not found after command | ||
| assert result.returncode == 1 | ||
| assert "unable to load or install binary nonexistent_binary_xyz123" in ( | ||
| assert "unable to install binary nonexistent_binary_xyz123" in ( |
There was a problem hiding this comment.
P3: Fix the expected stderr text to match the hook's actual failure message.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At abx_plugins/plugins/bash/tests/test_bash_provider.py, line 131:
<comment>Fix the expected stderr text to match the hook's actual failure message.</comment>
<file context>
@@ -128,7 +128,7 @@ def test_hook_fails_for_missing_binary_after_command(self):
# Should fail since binary not found after command
assert result.returncode == 1
- assert "unable to load or install binary nonexistent_binary_xyz123" in (
+ assert "unable to install binary nonexistent_binary_xyz123" in (
result.stderr.lower()
)
</file context>
| assert "unable to install binary nonexistent_binary_xyz123" in ( | |
| assert "nonexistent_binary_xyz123 not found after bash install" in ( |
… kwargs
The ``@anthropic-ai/claude-code`` npm package relies on a postinstall
script to fetch the native ``claude`` binary. With abxpkg's default
``postinstall_scripts=False`` security posture, npm is invoked with
``--ignore-scripts`` and the shim at ``.bin/claude`` just prints an
error ("claude native binary not installed"), which trips abxpkg's
"installed package did not produce runnable binary" verification and
rolls back the install.
- ``claudecode/config.json``: declare ``postinstall_scripts: true`` on
the ``{CLAUDECODE_BINARY}`` record so the npm lifecycle runs to
completion during dependency resolution.
- ``conftest.install_claude_code_with_hooks``: forward any top-level
binary-record fields (``postinstall_scripts``, ``min_release_age``,
etc.) as ``--key=value`` args so the npm hook's
``parse_extra_hook_args`` picks them up and threads them into
``Binary.model_validate``.
| # User pinned a literal cache directory — honour it exactly via the | ||
| # provider's ``browser_cache_dir`` override and co-locate ``bin_dir`` | ||
| # next to it. install_root is still the parent so other provider | ||
| # metadata (derived.env, npm helper dir) stays in a sibling tree. | ||
| # ``model_validate`` dispatches through pydantic so this works against | ||
| # both the current abxpkg (which accepts ``browser_cache_dir``) and | ||
| # older releases that still expose it as an accepted kwarg. | ||
| browser_cache_dir = Path(configured_cache_dir).expanduser().resolve() | ||
| browser_cache_dir.mkdir(parents=True, exist_ok=True) | ||
| provider = PuppeteerProvider( | ||
| browser_cache_dir=browser_cache_dir, | ||
| browser_bin_dir=browser_cache_dir.parent / "bin", | ||
| provider = PuppeteerProvider.model_validate( | ||
| { | ||
| "install_root": browser_cache_dir.parent, | ||
| "bin_dir": browser_cache_dir.parent / "bin", | ||
| "browser_cache_dir": browser_cache_dir, | ||
| }, |
There was a problem hiding this comment.
no do not attempt to auto-set bin_dir next to browser_cache_dir. honestly browser_cache_dir should never really be used by abx-plugins, we should leave it as the default and only manage install_root ourselves.
…root only Per maintainer feedback (#26): abx-plugins should only manage ``install_root`` and leave the provider's default ``browser_cache_dir`` (``install_root/cache``) alone. Removing the branch that tried to honour a user-configured ``PUPPETEER_CACHE_DIR`` via the provider's ``browser_cache_dir`` / ``bin_dir`` — the hook now always installs at ``<LIB_DIR>/puppeteer`` and delegates cache layout entirely to abxpkg.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="abx_plugins/plugins/puppeteer/on_BinaryRequest__12_puppeteer.py">
<violation number="1" location="abx_plugins/plugins/puppeteer/on_BinaryRequest__12_puppeteer.py:70">
P2: This change drops support for `PUPPETEER_CACHE_DIR`. The config option is still documented, but the new code always uses `lib_dir/puppeteer`, so user overrides are ignored.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| install_root = (Path(lib_dir) / "puppeteer").resolve() | ||
| install_root.mkdir(parents=True, exist_ok=True) | ||
| provider = PuppeteerProvider(install_root=install_root) | ||
| install_root = (Path(lib_dir) / "puppeteer").resolve() |
There was a problem hiding this comment.
P2: This change drops support for PUPPETEER_CACHE_DIR. The config option is still documented, but the new code always uses lib_dir/puppeteer, so user overrides are ignored.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At abx_plugins/plugins/puppeteer/on_BinaryRequest__12_puppeteer.py, line 70:
<comment>This change drops support for `PUPPETEER_CACHE_DIR`. The config option is still documented, but the new code always uses `lib_dir/puppeteer`, so user overrides are ignored.</comment>
<file context>
@@ -67,28 +67,9 @@ def main(
- install_root = (Path(lib_dir) / "puppeteer").resolve()
- install_root.mkdir(parents=True, exist_ok=True)
- provider = PuppeteerProvider(install_root=install_root)
+ install_root = (Path(lib_dir) / "puppeteer").resolve()
+ install_root.mkdir(parents=True, exist_ok=True)
+ provider = PuppeteerProvider(install_root=install_root)
</file context>
Companion to the previous commit: the puppeteer hook no longer honours ``PUPPETEER_CACHE_DIR`` as an install-root override (abx-plugins now manages only ``install_root`` and leaves browser_cache_dir at the provider default). Remove the stale property from ``config.json`` so the docs don't advertise a knob the plugin ignores.
2 participants
Summary
Re-aligns the plugin hook scripts with the current
abxpkgBinary/Provider API and fixes a post-install chmod that crashed on dangling symlinks inside the lib dir.provider.INSTALLER_BIN_ABSPATHaccesses. That attribute was removed fromBinProvider; the five hooks (apt/brew/cargo/npm/pip) now callprovider.INSTALLER_BINARY()inside atryso they still skip cleanly when the installer is missing.PuppeteerProvider(browser_cache_dir=…, browser_bin_dir=…)→PuppeteerProvider(install_root=…),ChromeWebstoreProvider(extensions_dir=…)→ChromeWebstoreProvider(bin_dir=…),NpmProvider(npm_prefix=…)→NpmProvider(install_root=…),PipProvider(pip_venv=…)→PipProvider(install_root=…).enforce_lib_permissionstolerates dangling symlinks. It was callingfp.stat()(follows symlinks) insideos.walk(lib_dir)and crashed withFileNotFoundErrorwhen a managed symlink pointed at a not-yet-installed target (e.g.lib/env/bin/uvbeforeuvis resolved). Now it useslstat()and skips symlinks — chmod on a symlink would follow it and fail the same way.pyproject.toml—exclude-newer-packageupdate. Addsabx-plugins = "1 second"to the override so the 14-day cutoff doesn't rescue an old PyPI release whose transitive requirements still mention the pre-renameabx-pkgpackage.Test plan
uv run prek run --all-files— cleanon_BinaryRequest__10_npm.py --name=puppeteer ...) — install completes, JSONL record emitted, post-installenforce_lib_permissionsno longer raises onlib/env/bin/uv.abxpkg+abx-dlbranches)Summary by cubic
Aligns plugin hooks with the current
abxpkgAPI, switches hooks/tests toBinary.install(), and hardens post-install chmod to skip dangling symlinks across the config tree. Enables@anthropic-ai/claude-codepostinstall and forwards extra Binary kwargs through the npm hook to fix the nativeclaudeinstall.Bug Fixes
enforce_lib_permissionsnow useslstat()and skips symlinks in bothlib/and non-libwalks (and top-level files) to avoid crashes on dangling links.Refactors
apt/brew/cargo/npm/piphooks gate onprovider.INSTALLER_BINARY()in a try/except to skip cleanly when installers aren’t present.load_or_install()withinstall()across hooks and plugin tests; update the bash provider test to expect the install-only error text; no change for already-installed binaries.NpmProvider/PipProvideruseinstall_root;ChromeWebstoreProviderusesbin_dir; puppeteer hook setsinstall_root=lib/puppeteerand lets the provider manage the cache dir; removePUPPETEER_CACHE_DIRfromplugins/puppeteer/config.json.pyproject.toml: addabx-plugins = "1 second"toexclude-newer-package.Written for commit 1c90004. Summary will update on new commits.