From a5548819bf605f189c9cef48e150c756d8d20db4 Mon Sep 17 00:00:00 2001 From: Avi Fenesh Date: Wed, 25 Feb 2026 17:35:29 +0200 Subject: [PATCH 1/4] fix(deps): auto-install missing dependencies on first run Add ensure-deps.js that detects missing playwright dependency and auto-installs it with lockfile-based coordination between processes. Call ensurePlaywright() in browser-launcher before requiring playwright. Move playwright from peerDependencies to dependencies in package.json so npm install resolves it automatically. Add missing_dependency error classification in web-ctl.js classifyError before the element_not_found branch to prevent 'Cannot find module' from being misclassified as an element not found error. Closes #66 --- .gitignore | 1 + package-lock.json | 9 +- package.json | 2 +- scripts/browser-launcher.js | 3 + scripts/ensure-deps.js | 177 ++++++++++++++++++++++++++++++++ scripts/web-ctl.js | 11 ++ tests/ensure-deps.test.js | 184 ++++++++++++++++++++++++++++++++++ tests/web-ctl-actions.test.js | 40 ++++++++ 8 files changed, 420 insertions(+), 7 deletions(-) create mode 100644 scripts/ensure-deps.js create mode 100644 tests/ensure-deps.test.js diff --git a/.gitignore b/.gitignore index 5272b58..fc1c75d 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ node_modules/ *.enc sessions/ +.deps-installing diff --git a/package-lock.json b/package-lock.json index 2649a8c..50b890c 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8,11 +8,11 @@ "name": "@agentsys/web-ctl", "version": "1.0.0", "license": "MIT", + "dependencies": { + "playwright": ">=1.40.0" + }, "bin": { "web-ctl": "scripts/web-ctl.js" - }, - "peerDependencies": { - "playwright": ">=1.40.0" } }, "node_modules/fsevents": { @@ -25,7 +25,6 @@ "os": [ "darwin" ], - "peer": true, "engines": { "node": "^8.16.0 || ^10.6.0 || >=11.0.0" } @@ -35,7 +34,6 @@ "resolved": "https://registry.npmjs.org/playwright/-/playwright-1.58.2.tgz", "integrity": "sha512-vA30H8Nvkq/cPBnNw4Q8TWz1EJyqgpuinBcHET0YVJVFldr8JDNiU9LaWAE1KqSkRYazuaBhTpB5ZzShOezQ6A==", "license": "Apache-2.0", - "peer": true, "dependencies": { "playwright-core": "1.58.2" }, @@ -54,7 +52,6 @@ "resolved": "https://registry.npmjs.org/playwright-core/-/playwright-core-1.58.2.tgz", "integrity": "sha512-yZkEtftgwS8CsfYo7nm0KE8jsvm6i/PTgVtB8DL726wNf6H2IMsDuxCpJj59KDaxCtSnrWan2AeDqM7JBaultg==", "license": "Apache-2.0", - "peer": true, "bin": { "playwright-core": "cli.js" }, diff --git a/package.json b/package.json index aca568e..a8ac3e4 100644 --- a/package.json +++ b/package.json @@ -9,7 +9,7 @@ "test": "node --test tests/*.test.js", "validate": "node scripts/validate.js" }, - "peerDependencies": { + "dependencies": { "playwright": ">=1.40.0" }, "author": { diff --git a/scripts/browser-launcher.js b/scripts/browser-launcher.js index 1bd9268..5db4a2c 100644 --- a/scripts/browser-launcher.js +++ b/scripts/browser-launcher.js @@ -3,6 +3,7 @@ const fs = require('fs'); const path = require('path'); const sessionStore = require('./session-store'); +const { ensurePlaywright } = require('./ensure-deps'); const IN_WSL = isWSL(); @@ -63,6 +64,7 @@ function cleanSingletonLock(profileDir) { * @returns {{ context, page }} */ async function launchBrowser(sessionName, options = {}) { + ensurePlaywright(); const { chromium } = require('playwright'); const profileDir = sessionStore.getProfileDir(sessionName); @@ -150,6 +152,7 @@ async function canLaunchHeaded() { const maxAttempts = 2; for (let attempt = 1; attempt <= maxAttempts; attempt++) { try { + ensurePlaywright(); const { chromium } = require('playwright'); const ctx = await chromium.launchPersistentContext('', { headless: false, diff --git a/scripts/ensure-deps.js b/scripts/ensure-deps.js new file mode 100644 index 0000000..1bedc4e --- /dev/null +++ b/scripts/ensure-deps.js @@ -0,0 +1,177 @@ +'use strict'; + +const path = require('path'); +const fs = require('fs'); +const { execSync } = require('child_process'); + +const PLUGIN_ROOT = path.resolve(__dirname, '..'); +const LOCKFILE = path.join(PLUGIN_ROOT, '.deps-installing'); +const LOCK_TIMEOUT_MS = 120_000; +const LOCK_POLL_MS = 1000; + +let _playwrightVerified = false; + +/** + * Check whether a process with the given PID is still running. + */ +function isProcessAlive(pid) { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} + +/** + * Acquire an exclusive lockfile for dependency installation. + * Waits up to LOCK_TIMEOUT_MS if another process holds the lock. + * Returns true if lock was acquired, false if another process finished first + * (meaning deps should now be available). + */ +function acquireLock() { + const start = Date.now(); + + while (true) { + try { + fs.writeFileSync(LOCKFILE, String(process.pid), { flag: 'wx' }); + return true; + } catch { + // Lock exists - check if holder is still alive + let holderPid; + try { + holderPid = parseInt(fs.readFileSync(LOCKFILE, 'utf8').trim(), 10); + } catch { + // Lock file disappeared between check and read - retry + continue; + } + + if (holderPid && !isProcessAlive(holderPid)) { + // Stale lock - remove and retry + try { fs.unlinkSync(LOCKFILE); } catch { /* race with another cleaner */ } + continue; + } + + // Another process is installing - wait + if (Date.now() - start > LOCK_TIMEOUT_MS) { + throw new Error( + `Timed out waiting for dependency installation by PID ${holderPid}. ` + + `Remove ${LOCKFILE} if the process is no longer running.` + ); + } + + // Busy-wait (sync context, cannot use setTimeout) + const waitUntil = Date.now() + LOCK_POLL_MS; + while (Date.now() < waitUntil) { /* spin */ } + } + } +} + +/** + * Release the installation lockfile. + */ +function releaseLock() { + try { fs.unlinkSync(LOCKFILE); } catch { /* already removed */ } +} + +/** + * Ensure playwright is available. Installs it automatically if missing. + * + * This function is synchronous. It uses a module-level cache so repeated + * calls within the same process are free after the first successful check. + * + * Set WEB_CTL_SKIP_AUTO_INSTALL=1 to disable auto-install (for CI/sandboxed + * environments). When set, throws an error with manual install instructions + * if playwright is missing. + * + * Note: execSync is used with hardcoded commands only (no user input), + * so shell injection is not a concern here. + */ +function ensurePlaywright() { + if (_playwrightVerified) return; + + try { + require.resolve('playwright', { paths: [PLUGIN_ROOT] }); + _playwrightVerified = true; + return; + } catch { + // playwright not found - proceed to install + } + + if (process.env.WEB_CTL_SKIP_AUTO_INSTALL === '1') { + throw new Error( + `Required dependency 'playwright' is not installed.\n` + + `Auto-install is disabled (WEB_CTL_SKIP_AUTO_INSTALL=1).\n` + + `Run manually:\n` + + ` cd ${PLUGIN_ROOT} && npm install && npx playwright install chromium` + ); + } + + const acquired = acquireLock(); + if (!acquired) { + // Another process finished installing - verify + try { + require.resolve('playwright', { paths: [PLUGIN_ROOT] }); + _playwrightVerified = true; + return; + } catch { + throw new Error( + `Dependency installation by another process did not resolve playwright.\n` + + `Run manually: cd ${PLUGIN_ROOT} && npm install && npx playwright install chromium` + ); + } + } + + try { + process.stderr.write('[web-ctl] Installing dependencies...\n'); + + execSync('npm install --production', { + cwd: PLUGIN_ROOT, + stdio: ['pipe', 'pipe', 'pipe'], + timeout: 60_000 + }); + + process.stderr.write('[web-ctl] Installing Chromium browser...\n'); + + execSync('npx playwright install chromium', { + cwd: PLUGIN_ROOT, + stdio: ['pipe', 'pipe', 'pipe'], + timeout: 120_000 + }); + + process.stderr.write('[web-ctl] Dependencies installed.\n'); + } catch (err) { + releaseLock(); + throw new Error( + `Automatic dependency installation failed: ${err.message}\n` + + `Run manually: cd ${PLUGIN_ROOT} && npm install && npx playwright install chromium` + ); + } + + releaseLock(); + + // Verify installation succeeded + try { + // Clear require cache so Node picks up newly installed module + delete require.cache[require.resolve('playwright', { paths: [PLUGIN_ROOT] })]; + } catch { /* ignore if cache clear fails */ } + + try { + require.resolve('playwright', { paths: [PLUGIN_ROOT] }); + _playwrightVerified = true; + } catch { + throw new Error( + `Playwright installed but still not resolvable from ${PLUGIN_ROOT}.\n` + + `Run manually: cd ${PLUGIN_ROOT} && npm install && npx playwright install chromium` + ); + } +} + +/** + * Reset the module-level cache. Exposed for testing only. + */ +function _resetCache() { + _playwrightVerified = false; +} + +module.exports = { ensurePlaywright, _resetCache }; diff --git a/scripts/web-ctl.js b/scripts/web-ctl.js index ebcaf2a..a3b70dc 100755 --- a/scripts/web-ctl.js +++ b/scripts/web-ctl.js @@ -822,6 +822,17 @@ function classifyError(err, { action, selector, snapshot } = {}) { }; } + // Missing dependency (must be before 'not found' check for element_not_found) + if (msg.includes('Cannot find module')) { + const moduleName = msg.match(/Cannot find module '([^']+)'/)?.[1] || 'unknown'; + const pluginDir = path.resolve(__dirname, '..'); + return { + error: 'missing_dependency', + message: `Required dependency not found: ${moduleName}`, + suggestion: `Run: cd ${pluginDir} && npm install && npx playwright install chromium` + }; + } + // Element not found / strict mode violation if (msg.includes('not found') || msg.includes('waiting for locator') || msg.includes('strict mode violation') || msg.includes('resolved to') || diff --git a/tests/ensure-deps.test.js b/tests/ensure-deps.test.js new file mode 100644 index 0000000..ece00ec --- /dev/null +++ b/tests/ensure-deps.test.js @@ -0,0 +1,184 @@ +'use strict'; + +const { describe, it, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); + +const PLUGIN_ROOT = path.resolve(__dirname, '..'); +const LOCKFILE = path.join(PLUGIN_ROOT, '.deps-installing'); + +function getModule() { + // Clear require cache so each test gets fresh module state + delete require.cache[require.resolve('../scripts/ensure-deps')]; + return require('../scripts/ensure-deps'); +} + +afterEach(() => { + // Clean up any leftover lockfiles + try { fs.unlinkSync(LOCKFILE); } catch { /* no lockfile */ } + // Restore env + delete process.env.WEB_CTL_SKIP_AUTO_INSTALL; +}); + +describe('ensurePlaywright', () => { + it('is a no-op when playwright is already available', () => { + const { ensurePlaywright } = getModule(); + // Should not throw - playwright is installed in dev environment + ensurePlaywright(); + }); + + it('module-level caching makes second call instant', () => { + const { ensurePlaywright } = getModule(); + // First call does the resolve check + ensurePlaywright(); + // Second call should return immediately from cache + const start = Date.now(); + ensurePlaywright(); + const elapsed = Date.now() - start; + assert.ok(elapsed < 5, `Second call took ${elapsed}ms, expected < 5ms`); + }); + + it('_resetCache allows re-verification', () => { + const { ensurePlaywright, _resetCache } = getModule(); + ensurePlaywright(); + _resetCache(); + // After reset, next call should re-check (still succeeds since playwright is installed) + ensurePlaywright(); + }); + + it('succeeds even with WEB_CTL_SKIP_AUTO_INSTALL=1 when dep is present', () => { + process.env.WEB_CTL_SKIP_AUTO_INSTALL = '1'; + const { ensurePlaywright } = getModule(); + // With playwright installed, this should succeed regardless of env var + // (the env var only blocks auto-install, not the resolve check) + ensurePlaywright(); + }); + + it('does not create lockfile when playwright is already available', () => { + const { ensurePlaywright } = getModule(); + ensurePlaywright(); + assert.equal(fs.existsSync(LOCKFILE), false, 'No lockfile when dep is present'); + }); +}); + +describe('ensure-deps source contracts', () => { + const source = fs.readFileSync( + path.join(__dirname, '..', 'scripts', 'ensure-deps.js'), + 'utf8' + ); + + it('checks WEB_CTL_SKIP_AUTO_INSTALL env var', () => { + assert.ok( + source.includes('WEB_CTL_SKIP_AUTO_INSTALL'), + 'Should check WEB_CTL_SKIP_AUTO_INSTALL env var' + ); + }); + + it('error message includes manual install instructions', () => { + assert.ok( + source.includes('npm install && npx playwright install chromium'), + 'Should include manual install instructions' + ); + }); + + it('uses require.resolve with plugin root paths', () => { + assert.ok( + source.includes("require.resolve('playwright'"), + 'Should use require.resolve to check for playwright' + ); + }); + + it('acquires lockfile before installation', () => { + assert.ok( + source.includes('.deps-installing'), + 'Should use .deps-installing lockfile' + ); + }); + + it('stores PID in lockfile for stale detection', () => { + assert.ok( + source.includes('process.pid'), + 'Should write process PID to lockfile' + ); + }); + + it('checks if lock holder process is alive', () => { + assert.ok( + source.includes('isProcessAlive'), + 'Should check if lock holder PID is alive' + ); + }); + + it('runs npm install --production during auto-install', () => { + assert.ok( + source.includes('npm install --production'), + 'Should run npm install --production' + ); + }); + + it('runs npx playwright install chromium during auto-install', () => { + assert.ok( + source.includes('npx playwright install chromium'), + 'Should install chromium browser' + ); + }); + + it('uses stdio pipe to avoid polluting JSON output', () => { + assert.ok( + source.includes("stdio: ['pipe', 'pipe', 'pipe']"), + 'Should pipe stdio to avoid polluting output' + ); + }); + + it('logs progress to stderr', () => { + assert.ok( + source.includes('process.stderr.write'), + 'Should log progress to stderr' + ); + }); +}); + +describe('lockfile management', () => { + it('no lockfile exists before ensurePlaywright', () => { + assert.equal(fs.existsSync(LOCKFILE), false, 'Lockfile should not exist initially'); + }); + + it('no lockfile left after successful ensurePlaywright', () => { + const { ensurePlaywright } = getModule(); + ensurePlaywright(); + assert.equal(fs.existsSync(LOCKFILE), false, 'Lockfile should be cleaned up'); + }); + + it('stale lockfile is not disturbed when dep is already available', () => { + // When playwright is already installed, ensurePlaywright returns before + // the lockfile path is ever reached. Verify a pre-existing lockfile is + // untouched (it would only matter during an install flow). + fs.writeFileSync(LOCKFILE, '999999'); + const { ensurePlaywright } = getModule(); + ensurePlaywright(); + // Lockfile is still there because ensurePlaywright returned early + // (playwright was already found). The afterEach hook cleans it up. + assert.ok(fs.existsSync(LOCKFILE), 'Lockfile untouched when dep already available'); + }); + + it('lockfile uses exclusive write flag', () => { + const source = fs.readFileSync( + path.join(__dirname, '..', 'scripts', 'ensure-deps.js'), + 'utf8' + ); + assert.ok(source.includes("flag: 'wx'"), 'Should use wx flag for atomic creation'); + }); +}); + +describe('ensure-deps exports', () => { + it('exports ensurePlaywright function', () => { + const mod = getModule(); + assert.equal(typeof mod.ensurePlaywright, 'function'); + }); + + it('exports _resetCache function', () => { + const mod = getModule(); + assert.equal(typeof mod._resetCache, 'function'); + }); +}); diff --git a/tests/web-ctl-actions.test.js b/tests/web-ctl-actions.test.js index c392a50..c21dd27 100644 --- a/tests/web-ctl-actions.test.js +++ b/tests/web-ctl-actions.test.js @@ -2,6 +2,7 @@ const { describe, it } = require('node:test'); const assert = require('node:assert/strict'); +const path = require('path'); // Extract classifyError for testing by requiring the module internals // Since classifyError is not exported, we test it via the error patterns it handles @@ -31,6 +32,17 @@ describe('error classification patterns', () => { }; } + // Missing dependency (must be before 'not found' check for element_not_found) + if (msg.includes('Cannot find module')) { + const moduleName = msg.match(/Cannot find module '([^']+)'/)?.[1] || 'unknown'; + const pluginDir = path.resolve(__dirname, '..'); + return { + error: 'missing_dependency', + message: `Required dependency not found: ${moduleName}`, + suggestion: `Run: cd ${pluginDir} && npm install && npx playwright install chromium` + }; + } + if (msg.includes('not found') || msg.includes('waiting for locator') || msg.includes('strict mode violation') || msg.includes('resolved to') || msg.includes('Timeout') && selector) { @@ -141,6 +153,34 @@ describe('error classification patterns', () => { const result = classifyError(new Error('First line\nSecond line\nThird line'), { action: 'click' }); assert.equal(result.message, 'First line'); }); + + it('classifies missing dependency errors', () => { + const result = classifyError( + new Error("Cannot find module 'playwright'"), + { action: 'goto' } + ); + assert.equal(result.error, 'missing_dependency'); + assert.ok(result.message.includes('playwright')); + assert.ok(result.suggestion.includes('npm install')); + }); + + it('missing dependency does NOT match element_not_found', () => { + const result = classifyError( + new Error("Cannot find module 'playwright'"), + { action: 'click', selector: 'css=button' } + ); + assert.equal(result.error, 'missing_dependency'); + assert.notEqual(result.error, 'element_not_found'); + }); + + it('extracts module name from Cannot find module error', () => { + const result = classifyError( + new Error("Cannot find module 'some-other-dep'"), + { action: 'goto' } + ); + assert.equal(result.error, 'missing_dependency'); + assert.ok(result.message.includes('some-other-dep')); + }); }); describe('click-wait action recognition', () => { From 1c3eec536a8839a4b8ed4e7df7997d00d01459de Mon Sep 17 00:00:00 2001 From: Avi Fenesh Date: Wed, 25 Feb 2026 17:44:48 +0200 Subject: [PATCH 2/4] fix: improve test quality and fix platform-specific test failures - Replace grep-based source contract tests with behavioral subprocess tests - Fix canLaunchHeaded tests that assumed Linux-only behavior (Windows can launch headed browsers without X11/DISPLAY) - Remove unnecessary defensive comment about shell injection in ensure-deps --- scripts/ensure-deps.js | 3 - tests/ensure-deps.test.js | 193 ++++++++++++++++------------------ tests/web-ctl-actions.test.js | 29 +++-- 3 files changed, 111 insertions(+), 114 deletions(-) diff --git a/scripts/ensure-deps.js b/scripts/ensure-deps.js index 1bedc4e..c28aa09 100644 --- a/scripts/ensure-deps.js +++ b/scripts/ensure-deps.js @@ -83,9 +83,6 @@ function releaseLock() { * Set WEB_CTL_SKIP_AUTO_INSTALL=1 to disable auto-install (for CI/sandboxed * environments). When set, throws an error with manual install instructions * if playwright is missing. - * - * Note: execSync is used with hardcoded commands only (no user input), - * so shell injection is not a concern here. */ function ensurePlaywright() { if (_playwrightVerified) return; diff --git a/tests/ensure-deps.test.js b/tests/ensure-deps.test.js index ece00ec..b95e9e3 100644 --- a/tests/ensure-deps.test.js +++ b/tests/ensure-deps.test.js @@ -2,6 +2,7 @@ const { describe, it, afterEach } = require('node:test'); const assert = require('node:assert/strict'); +const { execFileSync } = require('child_process'); const fs = require('fs'); const path = require('path'); @@ -9,30 +10,50 @@ const PLUGIN_ROOT = path.resolve(__dirname, '..'); const LOCKFILE = path.join(PLUGIN_ROOT, '.deps-installing'); function getModule() { - // Clear require cache so each test gets fresh module state delete require.cache[require.resolve('../scripts/ensure-deps')]; return require('../scripts/ensure-deps'); } +/** + * Run a script in a subprocess with Module._resolveFilename mocked + * to simulate playwright not being installed. + */ +function runWithMissingPlaywright(code, env = {}) { + const wrapper = ` + const Module = require('module'); + const orig = Module._resolveFilename; + Module._resolveFilename = function(request, ...args) { + if (request === 'playwright') { + const e = new Error("Cannot find module 'playwright'"); + e.code = 'MODULE_NOT_FOUND'; + throw e; + } + return orig.call(this, request, ...args); + }; + ${code} + `; + return execFileSync(process.execPath, ['-e', wrapper], { + cwd: PLUGIN_ROOT, + env: { ...process.env, ...env }, + timeout: 10_000, + encoding: 'utf8' + }); +} + afterEach(() => { - // Clean up any leftover lockfiles try { fs.unlinkSync(LOCKFILE); } catch { /* no lockfile */ } - // Restore env delete process.env.WEB_CTL_SKIP_AUTO_INSTALL; }); -describe('ensurePlaywright', () => { - it('is a no-op when playwright is already available', () => { +describe('ensurePlaywright - when playwright is available', () => { + it('succeeds without throwing', () => { const { ensurePlaywright } = getModule(); - // Should not throw - playwright is installed in dev environment ensurePlaywright(); }); it('module-level caching makes second call instant', () => { const { ensurePlaywright } = getModule(); - // First call does the resolve check ensurePlaywright(); - // Second call should return immediately from cache const start = Date.now(); ensurePlaywright(); const elapsed = Date.now() - start; @@ -43,131 +64,101 @@ describe('ensurePlaywright', () => { const { ensurePlaywright, _resetCache } = getModule(); ensurePlaywright(); _resetCache(); - // After reset, next call should re-check (still succeeds since playwright is installed) ensurePlaywright(); }); - it('succeeds even with WEB_CTL_SKIP_AUTO_INSTALL=1 when dep is present', () => { + it('succeeds with WEB_CTL_SKIP_AUTO_INSTALL=1 when dep is present', () => { process.env.WEB_CTL_SKIP_AUTO_INSTALL = '1'; const { ensurePlaywright } = getModule(); - // With playwright installed, this should succeed regardless of env var - // (the env var only blocks auto-install, not the resolve check) ensurePlaywright(); }); - it('does not create lockfile when playwright is already available', () => { + it('does not create lockfile when dep is already available', () => { const { ensurePlaywright } = getModule(); ensurePlaywright(); - assert.equal(fs.existsSync(LOCKFILE), false, 'No lockfile when dep is present'); + assert.equal(fs.existsSync(LOCKFILE), false); }); }); -describe('ensure-deps source contracts', () => { - const source = fs.readFileSync( - path.join(__dirname, '..', 'scripts', 'ensure-deps.js'), - 'utf8' - ); - - it('checks WEB_CTL_SKIP_AUTO_INSTALL env var', () => { - assert.ok( - source.includes('WEB_CTL_SKIP_AUTO_INSTALL'), - 'Should check WEB_CTL_SKIP_AUTO_INSTALL env var' - ); - }); - - it('error message includes manual install instructions', () => { - assert.ok( - source.includes('npm install && npx playwright install chromium'), - 'Should include manual install instructions' - ); - }); - - it('uses require.resolve with plugin root paths', () => { - assert.ok( - source.includes("require.resolve('playwright'"), - 'Should use require.resolve to check for playwright' - ); - }); - - it('acquires lockfile before installation', () => { - assert.ok( - source.includes('.deps-installing'), - 'Should use .deps-installing lockfile' - ); - }); - - it('stores PID in lockfile for stale detection', () => { - assert.ok( - source.includes('process.pid'), - 'Should write process PID to lockfile' - ); - }); - - it('checks if lock holder process is alive', () => { - assert.ok( - source.includes('isProcessAlive'), - 'Should check if lock holder PID is alive' - ); - }); - - it('runs npm install --production during auto-install', () => { - assert.ok( - source.includes('npm install --production'), - 'Should run npm install --production' - ); - }); - - it('runs npx playwright install chromium during auto-install', () => { - assert.ok( - source.includes('npx playwright install chromium'), - 'Should install chromium browser' - ); - }); - - it('uses stdio pipe to avoid polluting JSON output', () => { - assert.ok( - source.includes("stdio: ['pipe', 'pipe', 'pipe']"), - 'Should pipe stdio to avoid polluting output' - ); - }); - - it('logs progress to stderr', () => { - assert.ok( - source.includes('process.stderr.write'), - 'Should log progress to stderr' - ); +describe('ensurePlaywright - when playwright is missing', () => { + it('throws with install instructions when WEB_CTL_SKIP_AUTO_INSTALL=1', () => { + const code = ` + process.env.WEB_CTL_SKIP_AUTO_INSTALL = '1'; + const { ensurePlaywright } = require('./scripts/ensure-deps'); + try { + ensurePlaywright(); + process.stdout.write('NO_THROW'); + } catch (err) { + process.stdout.write(err.message); + } + `; + const output = runWithMissingPlaywright(code); + assert.notEqual(output, 'NO_THROW', 'Should have thrown'); + assert.ok(output.includes('playwright'), 'Should mention playwright'); + assert.ok(output.includes('WEB_CTL_SKIP_AUTO_INSTALL'), 'Should mention env var'); + assert.ok(output.includes('npm install'), 'Should include npm install command'); + assert.ok(output.includes('npx playwright install chromium'), 'Should include browser install'); + }); + + it('error message includes the plugin directory path', () => { + const code = ` + process.env.WEB_CTL_SKIP_AUTO_INSTALL = '1'; + const { ensurePlaywright } = require('./scripts/ensure-deps'); + try { + ensurePlaywright(); + } catch (err) { + process.stdout.write(err.message); + } + `; + const output = runWithMissingPlaywright(code); + assert.ok(output.includes('cd '), 'Should include cd command with plugin dir'); + }); + + it('error includes Run manually instructions', () => { + const code = ` + process.env.WEB_CTL_SKIP_AUTO_INSTALL = '1'; + const { ensurePlaywright } = require('./scripts/ensure-deps'); + try { + ensurePlaywright(); + } catch (err) { + process.stdout.write(err.message); + } + `; + const output = runWithMissingPlaywright(code); + assert.ok(output.includes('Run manually'), 'Should include run manually guidance'); }); }); describe('lockfile management', () => { it('no lockfile exists before ensurePlaywright', () => { - assert.equal(fs.existsSync(LOCKFILE), false, 'Lockfile should not exist initially'); + assert.equal(fs.existsSync(LOCKFILE), false); }); it('no lockfile left after successful ensurePlaywright', () => { const { ensurePlaywright } = getModule(); ensurePlaywright(); - assert.equal(fs.existsSync(LOCKFILE), false, 'Lockfile should be cleaned up'); + assert.equal(fs.existsSync(LOCKFILE), false); }); - it('stale lockfile is not disturbed when dep is already available', () => { - // When playwright is already installed, ensurePlaywright returns before - // the lockfile path is ever reached. Verify a pre-existing lockfile is - // untouched (it would only matter during an install flow). + it('pre-existing lockfile is untouched when dep is already available', () => { fs.writeFileSync(LOCKFILE, '999999'); const { ensurePlaywright } = getModule(); ensurePlaywright(); - // Lockfile is still there because ensurePlaywright returned early - // (playwright was already found). The afterEach hook cleans it up. assert.ok(fs.existsSync(LOCKFILE), 'Lockfile untouched when dep already available'); }); - it('lockfile uses exclusive write flag', () => { - const source = fs.readFileSync( - path.join(__dirname, '..', 'scripts', 'ensure-deps.js'), - 'utf8' + it('lockfile uses exclusive write flag for atomicity', () => { + fs.writeFileSync(LOCKFILE, String(process.pid), { flag: 'wx' }); + assert.throws( + () => fs.writeFileSync(LOCKFILE, 'other', { flag: 'wx' }), + { code: 'EEXIST' } ); - assert.ok(source.includes("flag: 'wx'"), 'Should use wx flag for atomic creation'); + }); + + it('lockfile contains PID for stale detection', () => { + fs.writeFileSync(LOCKFILE, String(process.pid), { flag: 'wx' }); + const content = fs.readFileSync(LOCKFILE, 'utf8').trim(); + assert.equal(content, String(process.pid)); }); }); diff --git a/tests/web-ctl-actions.test.js b/tests/web-ctl-actions.test.js index c21dd27..18c75fd 100644 --- a/tests/web-ctl-actions.test.js +++ b/tests/web-ctl-actions.test.js @@ -1122,9 +1122,13 @@ describe('auth wall headed checkpoint fix', () => { try { const launcher = require('../scripts/browser-launcher'); const result = await launcher.canLaunchHeaded(); - // Returns false (playwright probe fails in test env) but proves - // WAYLAND_DISPLAY alone is sufficient to pass the display check - assert.equal(result, false, 'should attempt probe with WAYLAND_DISPLAY'); + // On Linux without a real display, the probe fails and returns false. + // On Windows, headed browsers launch without X11, so probe may succeed. + if (process.platform === 'win32') { + assert.equal(typeof result, 'boolean', 'should return a boolean'); + } else { + assert.equal(result, false, 'should attempt probe with WAYLAND_DISPLAY'); + } } finally { if (origDisplay !== undefined) process.env.DISPLAY = origDisplay; else delete process.env.DISPLAY; @@ -1141,15 +1145,20 @@ describe('auth wall headed checkpoint fix', () => { try { const launcher = require('../scripts/browser-launcher'); const first = await launcher.canLaunchHeaded(); - assert.equal(first, false, 'first call without DISPLAY should return false'); - // Set DISPLAY and call again - if cached, it would still return false - // without reaching the probe. Instead it should try to require playwright, - // proving it re-evaluated the DISPLAY check (no stale cache). + if (process.platform === 'win32') { + // On Windows, headed browser can launch without DISPLAY + assert.equal(typeof first, 'boolean', 'first call should return a boolean'); + } else { + assert.equal(first, false, 'first call without DISPLAY should return false'); + } + // Set DISPLAY and call again - proves function re-evaluates env each time process.env.DISPLAY = ':99'; const second = await launcher.canLaunchHeaded(); - // Returns false (playwright probe fails in test env) but the point is - // it did NOT return a cached result from the first call - assert.equal(second, false, 'second call returns false (no real display)'); + if (process.platform === 'win32') { + assert.equal(typeof second, 'boolean', 'second call should return a boolean'); + } else { + assert.equal(second, false, 'second call returns false (no real display)'); + } } finally { if (origDisplay !== undefined) process.env.DISPLAY = origDisplay; else delete process.env.DISPLAY; From 34afd31be197cc64485a9f146e7fad5a4bcfdb20 Mon Sep 17 00:00:00 2001 From: Avi Fenesh Date: Wed, 25 Feb 2026 17:49:38 +0200 Subject: [PATCH 3/4] fix: replace busy-wait with Atomics.wait, add failure path tests - Replace CPU-spinning busy-wait loop with Atomics.wait (yields CPU) - Fix acquireLock to return false when lock disappears (was unreachable) - Add PID validation for NaN/invalid values in lockfile - Add subprocess tests for npm install failure, chromium install failure, lockfile cleanup after failure, and stale lock detection --- scripts/ensure-deps.js | 23 +++++---- tests/ensure-deps.test.js | 100 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 9 deletions(-) diff --git a/scripts/ensure-deps.js b/scripts/ensure-deps.js index c28aa09..301728d 100644 --- a/scripts/ensure-deps.js +++ b/scripts/ensure-deps.js @@ -23,11 +23,18 @@ function isProcessAlive(pid) { } } +/** + * Synchronous sleep that yields CPU (no busy-wait spin). + */ +function syncSleep(ms) { + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms); +} + /** * Acquire an exclusive lockfile for dependency installation. * Waits up to LOCK_TIMEOUT_MS if another process holds the lock. - * Returns true if lock was acquired, false if another process finished first - * (meaning deps should now be available). + * Returns true if lock was acquired, false if lock disappeared + * (another process finished installing). */ function acquireLock() { const start = Date.now(); @@ -42,12 +49,12 @@ function acquireLock() { try { holderPid = parseInt(fs.readFileSync(LOCKFILE, 'utf8').trim(), 10); } catch { - // Lock file disappeared between check and read - retry - continue; + // Lock file disappeared between check and read + return false; } - if (holderPid && !isProcessAlive(holderPid)) { - // Stale lock - remove and retry + if (!holderPid || isNaN(holderPid) || holderPid <= 0 || !isProcessAlive(holderPid)) { + // Stale or invalid lock - remove and retry try { fs.unlinkSync(LOCKFILE); } catch { /* race with another cleaner */ } continue; } @@ -60,9 +67,7 @@ function acquireLock() { ); } - // Busy-wait (sync context, cannot use setTimeout) - const waitUntil = Date.now() + LOCK_POLL_MS; - while (Date.now() < waitUntil) { /* spin */ } + syncSleep(LOCK_POLL_MS); } } } diff --git a/tests/ensure-deps.test.js b/tests/ensure-deps.test.js index b95e9e3..4e9ba1a 100644 --- a/tests/ensure-deps.test.js +++ b/tests/ensure-deps.test.js @@ -162,6 +162,106 @@ describe('lockfile management', () => { }); }); +describe('ensurePlaywright - install failure handling', () => { + it('reports npm install failure with manual instructions', () => { + // Mock both require.resolve (missing playwright) and execSync (npm fails) + const code = ` + const cp = require('child_process'); + const origExec = cp.execSync; + cp.execSync = function(cmd, opts) { + if (cmd.includes('npm install')) { + throw new Error('npm ERR! code ENETUNREACH'); + } + return origExec.call(this, cmd, opts); + }; + const { ensurePlaywright } = require('./scripts/ensure-deps'); + try { + ensurePlaywright(); + process.stdout.write('NO_THROW'); + } catch (err) { + process.stdout.write(err.message); + } + `; + const output = runWithMissingPlaywright(code); + assert.notEqual(output, 'NO_THROW', 'Should throw on npm failure'); + assert.ok(output.includes('installation failed'), 'Should mention failure'); + assert.ok(output.includes('Run manually'), 'Should include manual instructions'); + }); + + it('reports chromium install failure with manual instructions', () => { + // npm install succeeds but playwright install chromium fails + const code = ` + const cp = require('child_process'); + const origExec = cp.execSync; + cp.execSync = function(cmd, opts) { + if (cmd.includes('playwright install')) { + throw new Error('Failed to download chromium'); + } + return origExec.call(this, cmd, opts); + }; + const { ensurePlaywright } = require('./scripts/ensure-deps'); + try { + ensurePlaywright(); + process.stdout.write('NO_THROW'); + } catch (err) { + process.stdout.write(err.message); + } + `; + const output = runWithMissingPlaywright(code); + assert.notEqual(output, 'NO_THROW', 'Should throw on chromium install failure'); + assert.ok(output.includes('installation failed'), 'Should mention failure'); + assert.ok(output.includes('npx playwright install chromium'), 'Should include chromium install command'); + }); + + it('cleans up lockfile after install failure', () => { + const code = ` + const cp = require('child_process'); + const origExec = cp.execSync; + cp.execSync = function(cmd, opts) { + if (cmd.includes('npm install')) throw new Error('npm ERR!'); + return origExec.call(this, cmd, opts); + }; + const { ensurePlaywright } = require('./scripts/ensure-deps'); + try { ensurePlaywright(); } catch {} + const fs = require('fs'); + const path = require('path'); + const lockfile = path.join(__dirname, '.deps-installing'); + process.stdout.write(String(fs.existsSync(lockfile))); + `; + const output = runWithMissingPlaywright(code); + assert.equal(output, 'false', 'Lockfile should be cleaned up after failure'); + }); +}); + +describe('stale lock detection', () => { + it('cleans up lockfile with non-existent PID', () => { + // Use a subprocess to test stale lock detection + const code = ` + const fs = require('fs'); + const path = require('path'); + const lockfile = path.join(__dirname, '.deps-installing'); + // Create lockfile with PID that does not exist + fs.writeFileSync(lockfile, '2147483647', { flag: 'wx' }); + // Now when ensurePlaywright tries to install, acquireLock should + // detect stale lock, remove it, and acquire it + // Since we also mock execSync to fail, we'll see the install attempt + const cp = require('child_process'); + const origExec = cp.execSync; + cp.execSync = function(cmd, opts) { + if (cmd.includes('npm install')) throw new Error('expected'); + return origExec.call(this, cmd, opts); + }; + const { ensurePlaywright } = require('./scripts/ensure-deps'); + try { ensurePlaywright(); } catch (err) { + // It should have gotten past the stale lock and attempted install + process.stdout.write(err.message.includes('installation failed') ? 'STALE_CLEANED' : 'OTHER'); + } + `; + const output = runWithMissingPlaywright(code); + assert.equal(output, 'STALE_CLEANED', 'Should clean stale lock and attempt install'); + }); +}); + describe('ensure-deps exports', () => { it('exports ensurePlaywright function', () => { const mod = getModule(); From a1a40bf7b9eccf0a56ebaf1a8e02c7563e14d098 Mon Sep 17 00:00:00 2001 From: Avi Fenesh Date: Wed, 25 Feb 2026 17:56:41 +0200 Subject: [PATCH 4/4] docs: update install instructions for auto-dependency installation - README: replace manual playwright/chromium install with auto-install note - README: update Requirements section - CHANGELOG: add entry for auto-install feature - web-auth SKILL.md: update error handling to mention auto-install --- CHANGELOG.md | 1 + README.md | 10 +++++----- skills/web-auth/SKILL.md | 2 +- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c22c2ed..25c8369 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## [Unreleased] ### Added +- Auto-install missing dependencies (playwright and Chromium) on first browser operation - eliminates manual setup step. Lockfile-based coordination prevents race conditions. Set `WEB_CTL_SKIP_AUTO_INSTALL=1` to disable in CI/sandboxed environments - `--ensure-auth` flag for goto action - polls for auth completion at 2s intervals using URL-change heuristic instead of a static timed checkpoint. On success, closes headed browser, relaunches headless, and loads the original URL. Overrides `--no-auth-wall-detect` so auth detection runs even when wall detection is disabled - Auto-detect authentication walls after goto navigation - uses three-heuristic detection (domain cookies, URL auth patterns, DOM login elements) and automatically opens headed checkpoint. Disable with `--no-auth-wall-detect` flag - Smart default snapshot scoping - snapshots automatically scope to `
` element (then `[role="main"]`, fallback to ``), reducing output size by excluding navigation, headers, and footers. Use `--snapshot-full` to capture full page body when needed diff --git a/README.md b/README.md index 421d587..1d3205d 100644 --- a/README.md +++ b/README.md @@ -35,9 +35,9 @@ Each invocation is a single Node.js process. No daemon, no MCP server, no IPC. S # Claude Code agentsys install web-ctl -# Peer dependency -npm install playwright -npx playwright install chromium +# Dependencies auto-install on first use +# To disable auto-install (CI/sandboxed environments): +# export WEB_CTL_SKIP_AUTO_INSTALL=1 ``` ## Commands @@ -275,8 +275,8 @@ Can be invoked by: ## Requirements - Node.js 18+ -- Playwright (`npm install playwright`) -- Chromium (`npx playwright install chromium`) +- Playwright and Chromium (auto-installed on first browser operation) +- Set `WEB_CTL_SKIP_AUTO_INSTALL=1` to disable auto-install in CI/sandboxed environments ## License diff --git a/skills/web-auth/SKILL.md b/skills/web-auth/SKILL.md index 191d0a7..ea17b8b 100644 --- a/skills/web-auth/SKILL.md +++ b/skills/web-auth/SKILL.md @@ -117,7 +117,7 @@ If verification fails (`ok: false`), the auth flow still succeeds - the verifica On timeout: Ask the user if they want to retry with a longer timeout. On error: Check the error message. Common issues: -- Browser not found: User needs to install Chrome or Playwright (`npx playwright install chromium`) +- Browser not found: Dependencies should auto-install on first run. If disabled (`WEB_CTL_SKIP_AUTO_INSTALL=1`), install manually: `npm install && npx playwright install chromium` - Session locked: Another process is using this session ### 5. Verify Auth