diff --git a/src/webwright/run/doctor.py b/src/webwright/run/doctor.py index d64d5ed1..ac6029e9 100644 --- a/src/webwright/run/doctor.py +++ b/src/webwright/run/doctor.py @@ -1,8 +1,10 @@ from __future__ import annotations import os +import re import subprocess import sys +import tempfile from importlib.util import find_spec from pathlib import Path from rich.console import Console @@ -10,6 +12,14 @@ console = Console() +# Engines Webwright actually uses. Firefox first: the skill contract pins it +# because Akamai-fronted sites reject Playwright Chromium with +# ERR_HTTP2_PROTOCOL_ERROR on TLS/H2 fingerprinting. +BROWSER_ENGINES = ("firefox", "chromium") + +# Every model backend shipped under webwright/models/. +MODEL_BACKEND_ENV_VARS = ("OPENAI_API_KEY", "ANTHROPIC_API_KEY", "OPENROUTER_API_KEY") + def check_python(): version = sys.version_info @@ -27,77 +37,195 @@ def check_playwright(): return False, ("playwright not installed\nFix: pip install playwright") -def check_chromium(): +def _parse_install_locations(output: str) -> dict[str, Path]: + """Map each Playwright browser key to the directory it installs into. + + ``playwright install --dry-run`` prints a header line per browser followed + by an indented ``Install location:`` line, and it does so whether or not the + browser is actually present on disk. + """ + locations: dict[str, Path] = {} + current: str | None = None + + for line in output.splitlines(): + header = re.search(r"\(playwright ([a-z0-9-]+) v[^)]+\)", line) + + if header: + current = header.group(1) + continue + + if current and "Install location:" in line: + locations[current] = Path(line.split("Install location:", 1)[1].strip()) + current = None + + return locations + + +def check_browsers(): + """Verify that a Playwright engine is actually present on disk. + + Two bugs here previously. The command was invoked as a bare ``playwright`` + console script, which Windows cannot resolve -- surfacing a raw + ``[WinError 2]`` instead of an actionable message. And the check only looked + at the return code, which is 0 even when no browser is installed, so it + could never fail for the right reason. Parse the reported install locations + and stat them instead. + """ + if find_spec("playwright") is None: + return False, ("playwright not installed\nFix: pip install playwright") + try: result = subprocess.run( - ["playwright", "install", "--dry-run"], + [sys.executable, "-m", "playwright", "install", "--dry-run"], capture_output=True, text=True, + timeout=60, + ) + except Exception as e: + return False, ( + f"unable to query the Playwright driver: {e}\n" + "Fix: pip install playwright" + ) + + if result.returncode != 0: + return False, ( + "playwright driver unavailable\nFix: pip install playwright" ) - if result.returncode == 0: - return True, "chromium available" + locations = _parse_install_locations(result.stdout) - return False, ("chromium missing\nFix: playwright install chromium") + if not locations: + return False, ( + "could not parse 'playwright install --dry-run' output\n" + "Fix: playwright install firefox" + ) - except Exception as e: - return False, str(e) + found = [ + engine + for engine in BROWSER_ENGINES + if engine in locations and locations[engine].exists() + ] + missing = [engine for engine in BROWSER_ENGINES if engine not in found] + + if found: + detail = f"{', '.join(found)} available" + + if missing: + detail += f" (not installed: {', '.join(missing)})" + + return True, detail + + return False, ( + "no Playwright browsers installed\nFix: playwright install firefox" + ) def check_screenshot(): + """Validate real rendering with the engine the skill actually mandates. + + This previously hard-coded Chromium, so a correctly-provisioned Firefox-only + setup -- which is what ``skills/webwright/reference/playwright_patterns.md`` + tells users to install -- was reported as FAIL. It also wrote + ``doctor_test.png`` into the caller's working directory; use a temp dir. + """ try: from playwright.sync_api import sync_playwright + except ImportError: + return False, ("playwright not installed\nFix: pip install playwright") - screenshot_path = Path("doctor_test.png") + errors: list[str] = [] - with sync_playwright() as p: - browser = p.chromium.launch(headless=True) + with tempfile.TemporaryDirectory() as tmpdir: + screenshot_path = Path(tmpdir) / "doctor_test.png" - page = browser.new_page() + try: + with sync_playwright() as p: + for engine in BROWSER_ENGINES: + try: + browser = getattr(p, engine).launch(headless=True) + except Exception as e: + errors.append(f"{engine}: {type(e).__name__}") + continue - page.set_content("

Webwright Doctor

") + try: + page = browser.new_page() - page.screenshot(path=str(screenshot_path)) + page.set_content("

Webwright Doctor

") - browser.close() + page.screenshot(path=str(screenshot_path)) + finally: + browser.close() - if screenshot_path.exists(): - screenshot_path.unlink(missing_ok=True) + if screenshot_path.exists(): + return True, f"screenshot capture working ({engine})" - return True, "screenshot capture working" + errors.append(f"{engine}: screenshot file was not created") + except Exception as e: + errors.append(f"driver: {type(e).__name__}") - return False, "screenshot file was not created" + detail = f" [{'; '.join(errors)}]" if errors else "" + + return False, ( + f"unable to capture a screenshot with any installed browser{detail}\n" + "Fix: playwright install firefox" + ) - except Exception: - return False, ( - "unable to launch Chromium for screenshot validation\n" - "Fix: playwright install" - ) +def check_model_backend(): + """Accept any backend Webwright ships, and treat plugin mode as keyless. -def check_openai_key(): - if os.getenv("OPENAI_API_KEY"): - return True, "OPENAI_API_KEY found" + Hard-failing on a missing ``OPENAI_API_KEY`` reported FAIL for two entirely + valid configurations: an Anthropic/OpenRouter CLI run, and the Claude Code / + Codex plugin path, which needs no key because the host agent drives the loop. + """ + present = [name for name in MODEL_BACKEND_ENV_VARS if os.getenv(name)] + + if present: + return True, f"{', '.join(present)} found" return False, ( - "OPENAI_API_KEY missing\nFix: set the OPENAI_API_KEY environment variable" + f"no model API key found (checked {', '.join(MODEL_BACKEND_ENV_VARS)})\n" + "Fix: set one for CLI mode -- not required when running Webwright as a " + "Claude Code / Codex plugin" ) -def check_plugin_manifests(): - claude = Path(".claude-plugin/plugin.json") - codex = Path(".codex-plugin/plugin.json") +def _find_manifest_root(start: Path | None = None) -> Path | None: + """Walk upward from ``start`` looking for the plugin manifest directories. + + The manifests were resolved against the process working directory, so + ``webwright doctor`` reported FAIL from anywhere but the repo root. + """ + current = (start or Path.cwd()).resolve() + + for candidate in (current, *current.parents): + if (candidate / ".claude-plugin").is_dir() or (candidate / ".codex-plugin").is_dir(): + return candidate + + return None - missing = [] - if not claude.exists(): - missing.append("Claude") +def check_plugin_manifests(): + root = _find_manifest_root() - if not codex.exists(): - missing.append("Codex") + if root is None: + return False, ( + "missing plugin manifests: Claude, Codex\n" + "Fix: run doctor from inside the Webwright repo, or configure " + "Claude/Codex plugins" + ) + + missing = [ + label + for label, relative in ( + ("Claude", ".claude-plugin/plugin.json"), + ("Codex", ".codex-plugin/plugin.json"), + ) + if not (root / relative).is_file() + ] if not missing: - return True, "plugin manifests found" + return True, f"plugin manifests found ({root})" return False, ( f"missing plugin manifests: {', '.join(missing)}\n" @@ -108,9 +236,9 @@ def check_plugin_manifests(): CHECKS = [ ("Python", check_python), ("Playwright", check_playwright), - ("Chromium", check_chromium), + ("Browsers", check_browsers), ("Screenshot", check_screenshot), - ("OpenAI Key", check_openai_key), + ("Model Backend", check_model_backend), ("Plugins", check_plugin_manifests), ] diff --git a/tests/unit/test_doctor.py b/tests/unit/test_doctor.py index af2cdd14..b3359730 100644 --- a/tests/unit/test_doctor.py +++ b/tests/unit/test_doctor.py @@ -1,8 +1,8 @@ -from pathlib import Path - from webwright.run.doctor import ( - check_chromium, - check_openai_key, + _find_manifest_root, + _parse_install_locations, + check_browsers, + check_model_backend, check_playwright, check_plugin_manifests, check_python, @@ -24,13 +24,54 @@ def test_check_playwright(): assert isinstance(message, str) -def test_check_chromium(): - ok, message = check_chromium() +def test_parse_install_locations(): + """`--dry-run` lists every browser regardless of what is installed. + + The old check only inspected the return code, which is 0 even with no + browsers present, so parsing the locations is what makes the check real. + """ + sample = ( + "Chrome for Testing 151.0.7922.34 (playwright chromium v1234)\n" + " Install location: /tmp/ms-playwright/chromium-1234\n" + " Download url: https://example.invalid/chromium.zip\n" + "\n" + "Firefox 153.0 (playwright firefox v1538)\n" + " Install location: /tmp/ms-playwright/firefox-1538\n" + " Download url: https://example.invalid/firefox.zip\n" + ) + + locations = _parse_install_locations(sample) + + assert set(locations) == {"chromium", "firefox"} + assert locations["firefox"].name == "firefox-1538" + assert locations["chromium"].name == "chromium-1234" + + +def test_parse_install_locations_empty(): + assert _parse_install_locations("") == {} + + +def test_check_browsers(): + ok, message = check_browsers() assert isinstance(ok, bool) assert isinstance(message, str) +def test_check_browsers_reports_an_installed_engine(): + """A machine with any Playwright engine installed must not report FAIL. + + Regression: the old check shelled out to ``playwright install --dry-run``, + which exits 0 regardless, and raised ``[WinError 2]`` on Windows. + """ + ok, message = check_browsers() + + if ok: + assert "firefox" in message or "chromium" in message + else: + assert "Fix:" in message + + def test_check_screenshot(): ok, message = check_screenshot() @@ -38,22 +79,55 @@ def test_check_screenshot(): assert isinstance(message, str) -def test_check_openai_key_exists(monkeypatch): +def test_check_model_backend_accepts_openai(monkeypatch): + for name in ("ANTHROPIC_API_KEY", "OPENROUTER_API_KEY"): + monkeypatch.delenv(name, raising=False) + monkeypatch.setenv("OPENAI_API_KEY", "test-key") - ok, message = check_openai_key() + ok, message = check_model_backend() assert ok is True assert "found" in message -def test_check_openai_key_missing(monkeypatch): - monkeypatch.delenv("OPENAI_API_KEY", raising=False) +def test_check_model_backend_accepts_anthropic_only(monkeypatch): + """Regression: an Anthropic-only setup used to report FAIL. + + ``model_claude.yaml`` is a first-class backend, so requiring OPENAI_API_KEY + specifically made doctor lie about a valid configuration. + """ + for name in ("OPENAI_API_KEY", "OPENROUTER_API_KEY"): + monkeypatch.delenv(name, raising=False) + + monkeypatch.setenv("ANTHROPIC_API_KEY", "test-key") + + ok, message = check_model_backend() + + assert ok is True + assert "ANTHROPIC_API_KEY" in message + + +def test_check_model_backend_accepts_openrouter_only(monkeypatch): + for name in ("OPENAI_API_KEY", "ANTHROPIC_API_KEY"): + monkeypatch.delenv(name, raising=False) + + monkeypatch.setenv("OPENROUTER_API_KEY", "test-key") + + ok, message = check_model_backend() + + assert ok is True + assert "OPENROUTER_API_KEY" in message + + +def test_check_model_backend_missing_mentions_plugin_mode(monkeypatch): + for name in ("OPENAI_API_KEY", "ANTHROPIC_API_KEY", "OPENROUTER_API_KEY"): + monkeypatch.delenv(name, raising=False) - ok, message = check_openai_key() + ok, message = check_model_backend() assert ok is False - assert "missing" in message + assert "plugin" in message.lower() def test_plugin_manifests_exist(tmp_path, monkeypatch): @@ -83,12 +157,39 @@ def test_plugin_manifests_missing(tmp_path, monkeypatch): assert "missing" in message -def test_screenshot_file_cleanup(): - screenshot_path = Path("doctor_test.png") +def test_plugin_manifests_found_from_subdirectory(tmp_path, monkeypatch): + """Regression: doctor resolved manifests against cwd only. + + Running ``webwright doctor`` from any subdirectory of the repo reported the + manifests as missing. + """ + claude_dir = tmp_path / ".claude-plugin" + codex_dir = tmp_path / ".codex-plugin" + + claude_dir.mkdir() + codex_dir.mkdir() - if screenshot_path.exists(): - screenshot_path.unlink() + (claude_dir / "plugin.json").write_text("{}") + (codex_dir / "plugin.json").write_text("{}") + + nested = tmp_path / "src" / "webwright" / "run" + nested.mkdir(parents=True) + + monkeypatch.chdir(nested) + + assert _find_manifest_root() == tmp_path.resolve() + + ok, message = check_plugin_manifests() + + assert ok is True + assert "found" in message + + +def test_screenshot_does_not_pollute_cwd(tmp_path, monkeypatch): + """The temp screenshot must never be written into the caller's cwd.""" + monkeypatch.chdir(tmp_path) check_screenshot() - assert not screenshot_path.exists() + assert not (tmp_path / "doctor_test.png").exists() + assert list(tmp_path.iterdir()) == []