fix: install playwright-cli into a private prefix when npm's is root-owned - #8637
Open
RadiumGu wants to merge 1 commit into
Open
fix: install playwright-cli into a private prefix when npm's is root-owned#8637RadiumGu wants to merge 1 commit into
RadiumGu wants to merge 1 commit into
Conversation
…owned
On a host whose npm global prefix is not writable by the running user,
`npm install -g @playwright/cli` fails EACCES and the Browser panel's
install button could never succeed. The only exit was the standalone
installer, which the operator had to notice, copy and run themselves.
Measured on Amazon Linux where the prefix
/usr/local/node-v22.12.0-linux-arm64/lib/node_modules is owned by
`nobody`. A root-owned prefix is an ordinary host property -- a
distribution tarball unpacked into /usr/local, a system package, a
container image built as root -- not an operator mistake.
On EACCES/EPERM/EROFS only, retry into a prefix this user owns, reusing
npm's `--global --prefix` layout so the existing package-tree and
browser-revision probes need no new special case. Matched on npm's error
code rather than its prose, because its own advice ("run again as root")
is the one remedy that must not be taken: it would leave a root-owned
tree the gateway then cannot upgrade. A registry rejection is not
retried -- a different prefix cannot fix it, and retrying would hide the
real error behind a second one.
cli_path() and cli_env() now also search the private prefix bin dir,
appended so a system-wide install still wins when one exists. Without
that, the retry would SUCCEED while the panel kept reporting the CLI
absent. A best-effort ~/.local/bin symlink covers the agent, which runs
playwright-cli as a plain shell command and so resolves the login PATH;
it never replaces a non-symlink already there.
Verified end to end on the failing host: the global attempt reproduces
EACCES rc=243 and is classified, the retry succeeds, and install() now
completes all four steps with browser_ok true.
Contributor
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
1 similar comment
Collaborator
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
On a host whose npm global prefix is not writable by the running user, the Browser settings panel's install button could never succeed.
install()ran a barenpm install -g @playwright/cli@latestand returned on the first failure:Measured on Amazon Linux 2023 (arm64) where that prefix's
lib/node_modulesis owned bynobody:nobodymode0755. A root-owned npm prefix is an ordinary host property — a distribution tarball unpacked into/usr/local, a system package, a container image built as root — not an operator mistake. The only exit was the standalone installer, which the operator had to notice, copy and run in their own terminal.The module already had every piece needed to avoid this:
KIROCREW_PLAYWRIGHT_CLI_HOME,_standalone_node_modules()probing an unprivileged prefix, andplaywright-cli.shperforming exactly this install.install()just never used them.Change
On an
EACCES/EPERM/EROFSfailure only, retry into a prefix the user owns (<config dir>/playwright-cli), reusing npm's--global --prefixlayout so_standalone_node_modules()and_launcher_node_modules()need no new special case.Matched on npm's error code, not its prose. npm's own advice — "try running the command again as root/Administrator" — is the one remedy that must not be taken: it would leave a root-owned tree the gateway then cannot upgrade. A registry rejection or 403 is deliberately not retried: a different prefix cannot fix it, and retrying would repeat the failure while hiding the real error behind a second one.
Three supporting changes:
cli_path()andcli_env()also search the private prefix bin dir, appended so a system-widenpm install -gstill wins whenever one exists. Without this the retry would SUCCEED while the panel kept reporting the CLI absent — the failure shapecli_path()'s own docstring calls the worst available here.~/.local/binsymlink covers the agent, which is documented to runplaywright-clias a plain shell command and so resolves the login$PATH, which can never hold a KiroCrew-chosen prefix. Same conventionplaywright-cli.shfollows. It never replaces a non-symlink already there, and failure is logged rather than failing the step.stepslist so the operator sees what happened, but no longer vetoes an install the retry completed — the same pattern_download_browser()already uses for a refused--with-deps.Verification
End to end on the failing host:
npm-install-globalnpm-install-private-prefixinstall-browserinstall-skillsdetect()afterwards:installed: True,cli_version: 0.1.19,browser_ok: True.bash -lc 'playwright-cli --version'resolves through the symlink, and the browser-revision gate readschromium-1243through the package tree, confirming attribution survives the symlink chain.10 new tests; 277 passed / 3 skipped across
test_browser_cli_install.py,test_browser_cli_launch.py,test_browser_cli_os_deps.py,test_playwright_cli_installer.py,test_spawn_audit.py. Two existingcli_envPATH-composition assertions were updated for the appended layer.Not fixed here
One adjacent defect found while verifying, in
os_deps.pyrather thaninstall.py, and now fixed separately in #8639:install-browserreports ok on a host missing browser libraries.os_deps.host_deps_unsatisfiedkeys on Playwright's "Host system is missing dependencies" warning, which Playwright never emits here — its registry scanschrome-linuxwhile the arm64 build unpacks intochrome-linux-arm64, so validation finds nothing, calls the host good and writes a 30-dayDEPENDENCIES_VALIDATEDmarker. A false negative, not a missing warning.Correction to an earlier revision of this description: it also claimed
manual_deps_command()omitsmesa-libgbm. That was wrong — the package has always been in_RPM_CHROMIUM_PACKAGES(35 packages,mesa-libgbmincluded). The claim came from reading a truncatedrepr(...)[:400]of the generated command, which cuts off mid-token atlibxs, hiding the last four packages. No such defect exists.