fix: keep playwright-cli on PATH across node versions

[ashwin153]

Softlight Overview

UX Score: 5/5

Softlight reviewed this change and found no issues.

To run Softlight again, click here or comment @softlight.

npm install --global was the wrong home for the CLI. The node feature installs node through nvm, and nvm's global root is one directory per node version — the package landed in $NVM_DIR/versions/node/<active>/lib/node_modules with its shim in the matching bin, and only $NVM_DIR/current/bin is on PATH.

nvm install and nvm use both repoint current, so the CLI goes missing for the whole container, not just the shell that switched — current is a symlink on disk, so anything inheriting the feature's containerEnv PATH loses it too.

Reproduced against a real nvm install (node 22 → 20):

$ nvm use 20
Now using node v20.20.2
$ playwright-cli --version
bash: playwright-cli: command not found

Fix

Install under a dedicated prefix, /usr/local/share/playwright-cli, and symlink it to /usr/local/bin/playwright-cli. One copy, reachable from every node version — and from sudo, whose secure_path covers /usr/local/bin but never nvm. @playwright/cli declares node >=18, so it runs on whatever version is active.

Dropping the write into nvm's tree also lets the chown of the global root go; it was only there to undo root-owned files the install left in the remote user's node_modules.

Tests

The previous fix in this feature landed because every existing check passed against a broken image, so both new checks were verified to fail against the old install and pass against the new one:

• outside_nvm — the resolved binary is not under $NVM_DIR.
• survives_node_switch — installs a second node version, switches to it, and runs the CLI.

	old install	new install
outside nvm	FAIL	PASS
survives node switch	FAIL	PASS

pre-commit passes on the changed files. Feature version bumped to 1.2.0; the playwright:1 pin in the devcontainer repo picks it up with no change there.

---

Generated by Claude Code

---

Note

Medium Risk
Changes how and where the CLI is installed and owned, which can affect PATH resolution and upgrades for existing feature users. Regression tests cover the main failure mode.

Overview
Fixes playwright-cli disappearing from PATH after nvm use / nvm install.

Installs @playwright/cli under a dedicated prefix (/usr/local/share/playwright-cli) and symlinks it to /usr/local/bin/playwright-cli, instead of a plain npm install --global into nvm’s per-version global root. One copy stays reachable across node switches and via sudo.

Adds regression checks that the binary lives outside $NVM_DIR and still runs after switching to node 20. Documents the behavior and bumps the feature to 1.2.0.

^{Reviewed by Cursor Bugbot for commit a64c85d. Configure here.}

Greptile Summary

The PR moves playwright-cli out of nvm's version-specific global root so it remains available after Node version switches.

• Installs the CLI beneath /usr/local/share/playwright-cli and links it from /usr/local/bin.
• Adds placement, Node-switch, and remote-user writability regression checks.
• Documents Node-version behavior and unsupported sudo usage, updates the Node test feature to major version 2, and bumps the feature version to 1.2.0.

Confidence Score: 3/5

The PR is not yet safe to merge because an upgrade from the earlier user-owned shared prefix can leave the privileged CLI target writable by the remote user.

The reply shown as “Reply from :” claims that dropping INSTALL_PATH from chown fixed the issue, but current code neither restores root ownership nor clears inherited write permissions, so an existing prefix created by the earlier revision remains a concrete counterexample.

Files Needing Attention: src/playwright/install.sh and test/playwright/ubuntu.sh

Important Files Changed

Filename	Overview
src/playwright/install.sh	Moves the CLI to a shared prefix, but the ownership repair leaves prefixes made user-writable by an earlier PR revision writable across upgrades.
test/playwright/ubuntu.sh	Adds useful regression coverage, although its writability check does not cover writable directories or the executed module tree.
src/playwright/README.md	Documents the shared installation, Node-switch behavior, ownership intent, and unsupported sudo execution.
src/playwright/NOTES.md	Mirrors the detailed operational rationale and security boundary documented in the README.
test/playwright/scenarios.json	Updates the scenario to exercise the Node feature major version used by consumers.
src/playwright/devcontainer-feature.json	Bumps the feature version to 1.2.0 for the changed installation behavior.

_{Reviews (5): Last reviewed commit: "chore: drop the explanatory comments fro..." | Re-trigger Greptile}

Greptile also left 1 inline comment on this PR.

[ashwin153]

Correct, and reproduced. With node reachable only through nvm, the plain symlink dies exactly as described:

$ sudo -n /usr/local/bin/playwright-cli --version
/usr/bin/env: 'node': No such file or directory

Fixed in 4258f05. /usr/local/bin/playwright-cli is now a wrapper instead of a symlink. It fills in only what the caller is missing, so a normal shell keeps running the CLI on whichever node version it has active — which is the property this PR exists to protect:

if ! command -v node >/dev/null 2>&1; then
	PATH="${NVM_DIR:-/usr/local/share/nvm}/current/bin:${PATH}"
	export PATH
fi

One thing your report didn't mention that bites the same callers: sudo resets the environment, so it drops PLAYWRIGHT_BROWSERS_PATH from containerEnv too, and the browsers aren't where Playwright looks by default. The wrapper defaults that as well, again only when unset.

Added the sudo playwright-cli --version check you asked for. Verified it fails against the previous commit and passes against this one.

---

Generated by Claude Code

[ashwin153]

Right, and this one is my inconsistency rather than an edge case: I reasoned "sudo drops containerEnv" and then restored only one of the two variables containerEnv sets. Fixed in c6f0b02 — the wrapper now defaults PLAYWRIGHT_MCP_BROWSER alongside PLAYWRIGHT_BROWSERS_PATH, each still deferring to a caller-provided value:

--- empty env (sudo/cron simulation) ---
PLAYWRIGHT_MCP_BROWSER=chromium
PLAYWRIGHT_BROWSERS_PATH=/usr/local/share/ms-playwright
--- caller sets its own browser ---
PLAYWRIGHT_MCP_BROWSER=firefox
PLAYWRIGHT_BROWSERS_PATH=/custom

Your point about the test is the more useful half: --version never looks for a browser, so it could not have caught this. runs_under_sudo now opens and closes a session with no --browser flag, which is exactly the check that caught the same failure for ordinary shells in eee7eb1.

For the record, launching as root is safe here — playwright-core appends --no-sandbox unless chromiumSandbox: true is set explicitly, so the sudo path does not need special handling.

---

Generated by Claude Code

[ashwin153]

@greptileai @softlight Please re-review — both scores above are pinned to a64c85d, and the findings behind them were fixed in 4258f05.

• Softlight, UX 4/5 — "Running the CLI with sudo fails before it starts". /usr/local/bin/playwright-cli is now a wrapper, not a symlink; it resolves node (and PLAYWRIGHT_BROWSERS_PATH, which sudo also drops with containerEnv) only when the caller has neither.
• Greptile, Confidence 3/5 — the privileged-path target being user-writable. The chown of the install prefix is gone; it stays root:root with a+rX.

Both are now covered by checks that were confirmed to fail on a64c85d: runs_under_sudo and not_writable_by_remote_user. CI is green, with all four new checks passing in a real container.

---

Generated by Claude Code

[ashwin153]

@greptileai @softlight Re-review please — head is now c6f0b02, CI green.

Since the last scores (both pinned to a64c85d):

• 4258f05 — wrapper resolves node for sudo/cron; install prefix stays root:root (Greptile's P1).
• c6f0b02 — wrapper also restores PLAYWRIGHT_MCP_BROWSER, the second variable containerEnv sets, so a browser command from sudo/cron uses the bundled Chromium instead of the uninstalled chrome channel (Softlight's second finding).

Six checks now cover the feature, each confirmed to fail on the commit that introduced the defect it guards: placement outside $NVM_DIR, survival across a node version switch, running under sudo, opening a browser under sudo with no --browser flag, and the install prefix being read-only to the remote user.

Worth a specific look: everything found so far has been a distinct consequence of putting the CLI on a privileged path, so the sudo/cron paths are where I'd expect anything remaining to be.

---

Generated by Claude Code

[ashwin153]

Not taking this one, because the wrapper it applies to is gone as of 6c5f537 — but the finding is what pushed me to look at the wrapper properly, so it did its job.

Your analysis of the guard is correct: command -v node finding a node says nothing about it being new enough, and the package needs 18+. Adding the version check would have fixed that. The reason I removed the wrapper instead is that Greptile flagged two P1s on the same lines, and those are not fixable: the node feature does chown "${USERNAME}:nvm" "${NVM_DIR}", so the remote user owns the node binaries. A wrapper that prepends $NVM_DIR/current/bin for sudo has root running a user-replaceable interpreter no matter which version it selects — the guard would have made the wrapper more correct and left it unsafe.

sudo support was mine, not the bug this branch fixes, and it never worked before the branch. So /usr/local/bin/playwright-cli is a plain symlink again and the docs say sudo is unsupported. Both of your earlier findings stand and remain fixed.

---

Generated by Claude Code

[ashwin153]

@greptileai @softlight Re-review please — head is 6c5f537, CI green.

The change since your last pass is a removal, not a patch. Every finding after the first was on the sudo wrapper I had added, so the wrapper is gone:

• Both P1s — a wrapper that prepends $NVM_DIR/current/bin cannot be made safe here. The node feature runs chown "${USERNAME}:nvm" "${NVM_DIR}", so the remote user owns the node binaries; sudo would be root executing a user-replaceable interpreter regardless of which version it picked. Same for the Chromium tree, which is chowned to that user.
• The node-version guard would have made the wrapper more correct while leaving both P1s open, so it is declined rather than applied.

/usr/local/bin/playwright-cli is a plain symlink again and the docs state sudo is unsupported — which is what it was before this branch, so no regression.

What remains is the original fix, +22 lines in install.sh: @playwright/cli installs under /usr/local/share/playwright-cli instead of nvm's per-version global root, stays root:root with a+rX, and survives nvm use / nvm install. Three checks cover it — placement outside $NVM_DIR, survival across a node version switch, and the install being read-only to the remote user.

Greptile: your first finding (user-writable install prefix) is still fixed and still covered; it no longer depends on sudo to matter, since one install now backs every node version.

---

Generated by Claude Code

[ashwin153]

@greptileai @softlight Final re-review — head is 15fd1fe, CI green.

Since the last pass, no behavior changed. Two housekeeping commits:

• 7429f5b — the scenario pinned the node feature to major 1 while the feature's docs and the consuming devcontainer both ask for major 2, so the suite was not exercising the version anyone ships. Now on node:2, and the version-switch check passes against it.
• 15fd1fe — removed the explanatory comments I had added to install.sh and ubuntu.sh. Neither file carried comments before this branch; the rationale lives in NOTES.md and the commit messages.

The change under review is install.sh +11 −5: @playwright/cli installs under /usr/local/share/playwright-cli rather than nvm's per-version global root, is symlinked to /usr/local/bin/playwright-cli, and stays root:root with a+rX. Three checks cover it — placement outside $NVM_DIR, survival across nvm use, and the install being read-only to the remote user.

The sudo wrapper and everything it dragged in are gone; sudo is documented as unsupported, matching the behavior before this branch.

---

Generated by Claude Code

[greptile-apps]

Inherited writable CLI prefix

When rebuilding an image whose prefix was chowned to the remote user by the earlier implementation, chmod -R a+rX preserves that ownership and its existing write permissions while /usr/local/bin/playwright-cli continues to resolve into the writable tree, allowing a later privileged invocation to execute user-controlled launcher or module code. How this was verified: The current script excludes INSTALL_PATH from chown, and its additive chmod does not clear inherited write bits.

[ashwin153]

Taking the fix in f8098a8, though not at P1 — the mechanism is right and the severity is not.

The mechanism reproduces exactly as you describe. chmod -R a+rX only adds bits, so an existing prefix keeps its owner and its write permissions:

BEFORE: remote user CAN write the launcher
=== old line: chmod -R a+rX only ===
STILL WRITABLE
=== new lines ===
AFTER: reclaimed, not writable by remote user
still readable/executable by remote user

On severity: no published image can be in that state. No released version of this feature creates /usr/local/share/playwright-cli — main still installs into nvm's global root — and the workflow only publishes on push to main, so the implementation that chowned this prefix exists solely on unmerged commits of this PR. Reaching it means having built an image from a mid-review commit.

Worth fixing anyway, for a reason narrower than the one you gave: the script was getting root ownership by accident, because npm happened to create the directory as root, and not_writable_by_remote_user passed for that incidental reason rather than an enforced one. It now chowns to root:root and clears group/other write, so the install establishes the property instead of inheriting it.

One limit I'll state rather than paper over: the feature test builds from a clean base every time, so no check exercises the rebuild-over-existing-prefix path. The chown makes the outcome unconditional, but the test suite is not what demonstrates that.

---

Generated by Claude Code

[greptile-apps]

The fix is correct and complete. chown -R root:root "${INSTALL_PATH}" unconditionally reclaims ownership before chmod -R a+rX,go-w clears write bits for group/other — so the property is established by the script regardless of prior state, not inherited.

Your severity analysis holds: the vulnerable state (prefix chowned to remote user) can only exist in images built from unmerged mid-review commits, not from any published version of the feature. The fix is still the right call for the reason you named — the previous pass/fail of not_writable_by_remote_user was incidental, and now the script enforces what the test was checking.

Resolved from my side.