From ab1aaa2d8cb13630ec2d8a6fae00495dad16dca2 Mon Sep 17 00:00:00 2001 From: nhicks00 <93951766+nhicks00@users.noreply.github.com> Date: Sun, 1 Mar 2026 13:03:30 -0600 Subject: [PATCH 01/11] fix(cd): reload agent after /cd to flush stale system prompt and AGENT.md cache (#212) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(cd): reload agent after /cd to flush stale system prompt and AGENT.md cache After a successful /cd, the agent's PydanticAgent instance was reused verbatim for subsequent messages. This meant: 1. Any CWD-dependent content baked into the system prompt at agent construction time (e.g. the Walmart plugin's working-directory line) was never refreshed, so the LLM kept receiving the old path. 2. Project-local AGENT.md rules loaded by load_puppy_rules() were cached in self._puppy_rules and never re-read for the new directory, silently ignoring the new project's rules (or incorrectly carrying the old ones forward). Fixes: * base_agent.py — set self._puppy_rules = None at the top of reload_code_generation_agent() so every reload always performs a fresh disk read from the current working directory. * core_commands.py — call get_current_agent().reload_code_generation_agent() after os.chdir() succeeds. The reload is best-effort (wrapped in try/except) so a failure never aborts the directory change itself. Tests: * test_command_handler.py — update test_cd_valid_change to assert the agent is reloaded; add test_cd_valid_change_reload_failure_is_nonfatal to verify a broken reload does not bubble up. * test_base_agent_full_coverage.py — add test_reload_clears_puppy_rules_cache which uses a spy on load_puppy_rules() to prove _puppy_rules is None (i.e. cache cleared) at the moment the fresh read begins. Closes #205 * fix(cd): emit warning on agent reload failure instead of silent pass Address CodeRabbit review comments on PR #212: - Replace bare `except: pass` with `emit_warning()` so users see feedback when agent reload fails after /cd - Strengthen test assertions to verify chdir, success message, reload attempt, and warning emission Co-Authored-By: Claude Opus 4.6 (1M context) --------- Co-authored-by: n0h0123 Co-authored-by: Claude Opus 4.6 (1M context) --- code_puppy/agents/base_agent.py | 6 ++ code_puppy/command_line/core_commands.py | 16 ++++- tests/agents/test_base_agent_full_coverage.py | 63 +++++++++++++++++++ tests/test_command_handler.py | 44 +++++++++++++ 4 files changed, 128 insertions(+), 1 deletion(-) diff --git a/code_puppy/agents/base_agent.py b/code_puppy/agents/base_agent.py index 86ff147de..ac94d6e2e 100644 --- a/code_puppy/agents/base_agent.py +++ b/code_puppy/agents/base_agent.py @@ -1308,6 +1308,12 @@ def reload_code_generation_agent(self, message_group: Optional[str] = None): register_tools_for_agent, ) + # Invalidate the project-local rules cache so a fresh read from the + # current working directory is performed on the next load_puppy_rules() + # call. This is critical for /cd: the user may have switched to a + # different project that has its own AGENT.md (or none at all). + self._puppy_rules = None + if message_group is None: message_group = str(uuid.uuid4()) diff --git a/code_puppy/command_line/core_commands.py b/code_puppy/command_line/core_commands.py index 26360f6e1..0608baf89 100644 --- a/code_puppy/command_line/core_commands.py +++ b/code_puppy/command_line/core_commands.py @@ -55,7 +55,7 @@ def handle_cd_command(command: str) -> bool: # Use shlex.split to handle quoted paths properly import shlex - from code_puppy.messaging import emit_error, emit_info, emit_success + from code_puppy.messaging import emit_error, emit_info, emit_success, emit_warning try: tokens = shlex.split(command) @@ -77,6 +77,20 @@ def handle_cd_command(command: str) -> bool: if os.path.isdir(target): os.chdir(target) emit_success(f"Changed directory to: {target}") + # Reload the agent so the system prompt and project-local + # AGENT.md rules reflect the new working directory. Without + # this, the LLM keeps receiving stale path information for the + # remainder of the session (the PydanticAgent instructions are + # baked in at construction time and never refreshed otherwise). + try: + from code_puppy.agents.agent_manager import get_current_agent + + get_current_agent().reload_code_generation_agent() + except Exception as e: + emit_warning( + f"Directory changed, but agent reload failed: {e}. " + "You may need to run /agent or /model to force a refresh." + ) else: emit_error(f"Not a directory: {dirname}") return True diff --git a/tests/agents/test_base_agent_full_coverage.py b/tests/agents/test_base_agent_full_coverage.py index 875c68e58..4165df12e 100644 --- a/tests/agents/test_base_agent_full_coverage.py +++ b/tests/agents/test_base_agent_full_coverage.py @@ -940,6 +940,69 @@ def test_dbos_reload( agent.reload_code_generation_agent() mock_dbos_agent.assert_called() + def test_reload_clears_puppy_rules_cache(self, agent): + """reload_code_generation_agent must invalidate the _puppy_rules cache. + + This ensures that /cd picks up the new project's AGENT.md rules instead + of keeping the old directory's rules baked in for the session. + """ + # Seed a stale cached value as if a previous directory's AGENT.md was loaded. + stale_rules = "old project rules" + agent._puppy_rules = stale_rules + + # Capture the _puppy_rules value at the moment load_puppy_rules() is + # entered to prove it was cleared *before* the fresh read. + value_at_load_time = [] + original_load = agent.load_puppy_rules + + def spy_load(): + # _puppy_rules should already be None when we arrive here + value_at_load_time.append(agent._puppy_rules) + return original_load() + + with ( + patch.object(agent, "load_puppy_rules", side_effect=spy_load), + patch.object(agent, "get_model_name", return_value="model"), + patch( + "code_puppy.agents.base_agent.ModelFactory.load_config", + return_value={"model": {}}, + ), + patch( + "code_puppy.agents.base_agent.ModelFactory.get_model", + return_value=MagicMock(), + ), + patch( + "code_puppy.agents.base_agent.get_agent_pinned_model", + return_value=None, + ), + patch( + "code_puppy.agents.base_agent.get_mcp_manager", + ) as mock_mcp, + patch( + "code_puppy.model_utils.prepare_prompt_for_model", + return_value=MagicMock(instructions="test"), + ), + patch( + "code_puppy.agents.base_agent.PydanticAgent", + return_value=MagicMock(_tools={}), + ), + patch("code_puppy.tools.register_tools_for_agent"), + patch("code_puppy.tools.has_extended_thinking_active", return_value=False), + patch("code_puppy.agents.base_agent.get_use_dbos", return_value=False), + patch("code_puppy.agents.base_agent.make_model_settings", return_value={}), + ): + mock_mcp.return_value.get_servers_for_agent.return_value = [] + agent.reload_code_generation_agent() + + # The spy must have been called (load_puppy_rules was invoked during reload). + assert value_at_load_time, "load_puppy_rules was never called during reload" + # Crucially, _puppy_rules must have been None at entry to load_puppy_rules, + # proving the cache was cleared before the fresh disk read. + assert value_at_load_time[0] is None, ( + f"_puppy_rules was not cleared before re-loading rules; " + f"got: {value_at_load_time[0]!r}" + ) + class TestCreateAgentWithOutputType: """Tests for _create_agent_with_output_type (lines 1603-1657).""" diff --git a/tests/test_command_handler.py b/tests/test_command_handler.py index 0368ca090..ed7459daa 100644 --- a/tests/test_command_handler.py +++ b/tests/test_command_handler.py @@ -59,24 +59,68 @@ def test_cd_show_lists_directories(): def test_cd_valid_change(): + """Successful /cd must chdir, emit success, and reload the agent.""" mocks = setup_messaging_mocks() mock_emit_success = mocks["emit_success"].start() try: + mock_agent = MagicMock() with ( patch("os.path.expanduser", side_effect=lambda x: x), patch("os.path.isabs", return_value=True), patch("os.path.isdir", return_value=True), patch("os.chdir") as mock_chdir, + patch( + "code_puppy.agents.agent_manager.get_current_agent", + return_value=mock_agent, + ), ): result = handle_command("/cd /some/dir") assert result is True mock_chdir.assert_called_once_with("/some/dir") mock_emit_success.assert_called_with("Changed directory to: /some/dir") + # Agent must be reloaded so the system prompt and AGENT.md rules + # reflect the new working directory. + mock_agent.reload_code_generation_agent.assert_called_once() finally: mocks["emit_success"].stop() +def test_cd_valid_change_reload_failure_is_nonfatal(): + """A reload failure after /cd must not abort the directory change.""" + mocks = setup_messaging_mocks() + mock_emit_success = mocks["emit_success"].start() + mock_emit_warning = mocks["emit_warning"].start() + + try: + mock_agent = MagicMock() + mock_agent.reload_code_generation_agent.side_effect = Exception("boom") + with ( + patch("os.path.expanduser", side_effect=lambda x: x), + patch("os.path.isabs", return_value=True), + patch("os.path.isdir", return_value=True), + patch("os.chdir") as mock_chdir, + patch( + "code_puppy.agents.agent_manager.get_current_agent", + return_value=mock_agent, + ), + ): + # Should not raise even though reload raises. + result = handle_command("/cd /some/dir") + assert result is True + mock_chdir.assert_called_once_with("/some/dir") + mock_emit_success.assert_called_once_with("Changed directory to: /some/dir") + mock_agent.reload_code_generation_agent.assert_called_once() + # Reload failure should emit a warning, not silently pass + mock_emit_warning.assert_called_once() + warning_msg = str(mock_emit_warning.call_args) + assert "agent reload failed" in warning_msg + assert "boom" in warning_msg + finally: + mocks["emit_success"].stop() + mocks["emit_warning"].stop() + + def test_cd_invalid_directory(): mocks = setup_messaging_mocks() mock_emit_error = mocks["emit_error"].start() From dfe8df00177f5dd67fe77c307a238811195f834a Mon Sep 17 00:00:00 2001 From: Mike Pfaffenberger Date: Sun, 1 Mar 2026 15:09:18 -0500 Subject: [PATCH 02/11] fix: remove non-existent banner_color_mcp_tool_call from test expectations The DEFAULT_BANNER_COLORS dict in config.py doesn't have 'mcp_tool_call', so the tests were failing. Updated both test cases in TestGetConfigKeys to match the actual implementation. --- tests/test_config.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/test_config.py b/tests/test_config.py index 5fa231bbd..406944a11 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -299,7 +299,6 @@ def test_get_config_keys_with_existing_keys( "banner_color_subagent_response", "banner_color_terminal_tool", "banner_color_thinking", - "banner_color_mcp_tool_call", "banner_color_universal_constructor", "cancel_agent_key", "compaction_strategy", @@ -354,7 +353,6 @@ def test_get_config_keys_empty_config( "banner_color_subagent_response", "banner_color_terminal_tool", "banner_color_thinking", - "banner_color_mcp_tool_call", "banner_color_universal_constructor", "cancel_agent_key", "compaction_strategy", From 8de83d73eb5e9423145b93871d3a15a657ef5368 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Sun, 1 Mar 2026 20:22:47 +0000 Subject: [PATCH 03/11] chore: bump version [ci skip] --- pyproject.toml | 2 +- uv.lock | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 86e625758..ad4b00274 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "code-puppy" -version = "0.0.424" +version = "0.0.425" description = "Code generation agent" readme = "README.md" requires-python = ">=3.11,<3.14" diff --git a/uv.lock b/uv.lock index 898d5dfad..189ad0f0a 100644 --- a/uv.lock +++ b/uv.lock @@ -189,7 +189,7 @@ wheels = [ [[package]] name = "code-puppy" -version = "0.0.424" +version = "0.0.425" source = { editable = "." } dependencies = [ { name = "anthropic" }, From 847fc0054d6cf47441fed70d1f4e5dac3c627fa6 Mon Sep 17 00:00:00 2001 From: tdarei Date: Mon, 2 Mar 2026 20:39:58 +0000 Subject: [PATCH 04/11] Add Lightpanda browser backend and MCP catalog option --- code_puppy/mcp_/server_registry_catalog.py | 28 +++ code_puppy/tools/browser/browser_control.py | 2 +- code_puppy/tools/browser/browser_manager.py | 208 +++++++++++++++++- .../browser/test_browser_manager_full.py | 84 +++++++ 4 files changed, 316 insertions(+), 6 deletions(-) diff --git a/code_puppy/mcp_/server_registry_catalog.py b/code_puppy/mcp_/server_registry_catalog.py index 67a08ceaf..76a61dad3 100644 --- a/code_puppy/mcp_/server_registry_catalog.py +++ b/code_puppy/mcp_/server_registry_catalog.py @@ -460,6 +460,34 @@ def to_server_config(self, custom_name: Optional[str] = None, **cmd_args) -> Dic ), ), # ========== Web & Browser ========== + MCPServerTemplate( + id="lightpanda", + name="lightpanda", + display_name="Lightpanda Browser", + description="Headless browser automation using a Lightpanda CDP endpoint", + category="Web", + tags=["browser", "web", "scraping", "automation", "lightpanda", "cdp"], + type="stdio", + config={ + "command": "npx", + "args": ["-y", "openclaw-lightpanda-mcp"], + "env": {"LIGHTPANDA_CDP_URL": "${cdp_url}"}, + "timeout": 60, + }, + requires=MCPServerRequirements( + command_line_args=[ + { + "name": "cdp_url", + "prompt": "Lightpanda CDP URL", + "default": "ws://127.0.0.1:9222", + "required": False, + } + ], + required_tools=["node", "npm", "npx", "lightpanda"], + package_dependencies=["openclaw-lightpanda-mcp"], + system_requirements=["Running Lightpanda CDP endpoint"], + ), + ), MCPServerTemplate( id="puppeteer", name="puppeteer", diff --git a/code_puppy/tools/browser/browser_control.py b/code_puppy/tools/browser/browser_control.py index 25d1e35c4..9c370c1ae 100644 --- a/code_puppy/tools/browser/browser_control.py +++ b/code_puppy/tools/browser/browser_control.py @@ -218,7 +218,7 @@ async def browser_initialize( Args: headless: Run browser in headless mode (no GUI) - browser_type: Browser engine (chromium, firefox, webkit) + browser_type: Browser engine (chromium, firefox, webkit, lightpanda) homepage: Initial page to load Returns: diff --git a/code_puppy/tools/browser/browser_manager.py b/code_puppy/tools/browser/browser_manager.py index d1b4da07d..f5828123a 100644 --- a/code_puppy/tools/browser/browser_manager.py +++ b/code_puppy/tools/browser/browser_manager.py @@ -1,12 +1,16 @@ -"""Playwright browser manager for browser automation. +"""Playwright-compatible browser manager for browser automation. Supports multiple simultaneous instances with unique profile directories. """ import asyncio import atexit +import contextlib import contextvars import os +import shlex +import shutil +import socket from pathlib import Path from typing import Callable, Dict, Optional @@ -97,14 +101,18 @@ def get_session_browser_manager() -> "BrowserManager": class BrowserManager: - """Browser manager for Playwright-based browser automation. + """Browser manager for browser automation. Supports multiple simultaneous instances, each with its own profile directory. - Uses Chromium by default for maximum compatibility. + Uses Playwright Chromium by default for maximum compatibility. + Supports Lightpanda as an optional CDP backend. """ _browser: Optional[Browser] = None _context: Optional[BrowserContext] = None + _playwright: Optional[object] = None + _lightpanda_process: Optional[asyncio.subprocess.Process] = None + _lightpanda_endpoint: Optional[str] = None _initialized: bool = False def __init__( @@ -143,13 +151,184 @@ def _get_profile_directory(self) -> Path: profile_path.mkdir(parents=True, exist_ok=True, mode=0o700) return profile_path + @staticmethod + def _find_free_port() -> int: + """Find an available local TCP port.""" + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as sock: + sock.bind(("127.0.0.1", 0)) + return int(sock.getsockname()[1]) + + def _resolve_lightpanda_executable(self) -> str: + """Resolve the Lightpanda executable path from env or PATH.""" + executable = os.getenv("LIGHTPANDA_EXECUTABLE", "lightpanda") + + path_like = os.path.sep in executable or ( + os.path.altsep is not None and os.path.altsep in executable + ) + + if path_like: + if Path(executable).exists(): + return executable + elif shutil.which(executable): + return executable + + raise RuntimeError( + "Lightpanda executable not found. Install Lightpanda or set " + "LIGHTPANDA_EXECUTABLE to a valid path." + ) + + def _get_lightpanda_host(self) -> str: + """Get Lightpanda host.""" + return os.getenv("LIGHTPANDA_HOST", "127.0.0.1") + + def _get_lightpanda_port(self) -> int: + """Get Lightpanda CDP port from env or an ephemeral free port.""" + configured_port = os.getenv("LIGHTPANDA_PORT") + if configured_port: + try: + return int(configured_port) + except ValueError as exc: + raise RuntimeError( + f"Invalid LIGHTPANDA_PORT value: {configured_port}" + ) from exc + + return self._find_free_port() + + @staticmethod + def _get_lightpanda_startup_timeout() -> float: + """Get Lightpanda startup timeout in seconds.""" + timeout_raw = os.getenv("LIGHTPANDA_STARTUP_TIMEOUT", "10") + try: + timeout = float(timeout_raw) + except ValueError as exc: + raise RuntimeError( + f"Invalid LIGHTPANDA_STARTUP_TIMEOUT value: {timeout_raw}" + ) from exc + return max(timeout, 1.0) + + async def _read_lightpanda_stderr(self) -> str: + """Read Lightpanda stderr if available for better startup errors.""" + if not self._lightpanda_process or not self._lightpanda_process.stderr: + return "" + + with contextlib.suppress(Exception): + data = await self._lightpanda_process.stderr.read() + if data: + return data.decode(errors="replace").strip()[:500] + + return "" + + def _build_lightpanda_command(self, host: str, port: int) -> list[str]: + """Build the Lightpanda startup command.""" + executable = self._resolve_lightpanda_executable() + command = [ + executable, + "serve", + f"--host={host}", + f"--port={port}", + ] + + extra_args_raw = os.getenv("LIGHTPANDA_ARGS", "").strip() + if extra_args_raw: + command.extend(shlex.split(extra_args_raw, posix=os.name != "nt")) + + return command + + async def _connect_lightpanda_over_cdp(self, endpoint: str) -> Browser: + """Connect Playwright to Lightpanda CDP with retry.""" + timeout_s = self._get_lightpanda_startup_timeout() + loop = asyncio.get_running_loop() + deadline = loop.time() + timeout_s + last_error: Optional[Exception] = None + + while loop.time() < deadline: + if ( + self._lightpanda_process + and self._lightpanda_process.returncode is not None + ): + stderr_text = await self._read_lightpanda_stderr() + raise RuntimeError( + "Lightpanda process exited before CDP connection was ready " + f"(code={self._lightpanda_process.returncode}). " + f"{stderr_text}" + ) + + try: + if not self._playwright: + raise RuntimeError("Playwright is not initialized.") + return await self._playwright.chromium.connect_over_cdp(endpoint) + except Exception as exc: + last_error = exc + await asyncio.sleep(0.2) + + raise RuntimeError( + f"Timed out connecting to Lightpanda CDP at {endpoint}: {last_error}" + ) + + async def _initialize_lightpanda_browser(self) -> None: + """Initialize Lightpanda and attach Playwright over CDP.""" + from playwright.async_api import async_playwright + + if not self.headless: + emit_warning( + "Lightpanda is headless-only; forcing headless mode for this session." + ) + self.headless = True + + host = self._get_lightpanda_host() + port = self._get_lightpanda_port() + self._lightpanda_endpoint = f"http://{host}:{port}" + + command = self._build_lightpanda_command(host, port) + emit_info(f"Starting Lightpanda CDP endpoint at {host}:{port}") + + self._lightpanda_process = await asyncio.create_subprocess_exec( + *command, + stdout=asyncio.subprocess.DEVNULL, + stderr=asyncio.subprocess.PIPE, + ) + + self._playwright = await async_playwright().start() + browser = await self._connect_lightpanda_over_cdp(self._lightpanda_endpoint) + + # Reuse existing context when available (CDP default context), + # otherwise create one for consistent manager behavior. + if browser.contexts: + context = browser.contexts[0] + else: + context = await browser.new_context() + + self._browser = browser + self._context = context + + async def _stop_lightpanda_process(self) -> None: + """Stop Lightpanda process if this manager started one.""" + if not self._lightpanda_process: + return + + process = self._lightpanda_process + self._lightpanda_process = None + self._lightpanda_endpoint = None + + if process.returncode is None: + process.terminate() + try: + await asyncio.wait_for(process.wait(), timeout=3) + except asyncio.TimeoutError: + process.kill() + with contextlib.suppress(Exception): + await process.wait() + async def async_initialize(self) -> None: - """Initialize Chromium browser via Playwright.""" + """Initialize a browser backend.""" if self._initialized: return try: - emit_info(f"Initializing Chromium browser (session: {self.session_id})...") + browser_name = self.browser_type or "chromium" + emit_info( + f"Initializing {browser_name} browser (session: {self.session_id})..." + ) await self._initialize_browser() self._initialized = True @@ -178,6 +357,12 @@ async def _initialize_browser(self) -> None: self._initialized = True return + if self.browser_type == "lightpanda": + emit_info(f"Using Lightpanda browser (session: {self.session_id})") + await self._initialize_lightpanda_browser() + self._initialized = True + return + # Default: use Playwright Chromium from playwright.async_api import async_playwright @@ -258,6 +443,19 @@ async def _cleanup(self, silent: bool = False) -> None: pass # Ignore errors during browser close self._browser = None + if self._playwright: + try: + await self._playwright.stop() + except Exception: + pass # Ignore errors during playwright shutdown + self._playwright = None + + if self._lightpanda_process: + try: + await self._stop_lightpanda_process() + except Exception: + pass # Ignore errors during Lightpanda shutdown + self._initialized = False # Remove from active managers diff --git a/tests/tools/browser/test_browser_manager_full.py b/tests/tools/browser/test_browser_manager_full.py index 975b8e3f0..667de4d74 100644 --- a/tests/tools/browser/test_browser_manager_full.py +++ b/tests/tools/browser/test_browser_manager_full.py @@ -125,6 +125,65 @@ async def custom_init(manager): del bm._CUSTOM_BROWSER_TYPES["custom"] + @pytest.mark.asyncio + async def test_initialize_browser_lightpanda_type(self): + from code_puppy.tools.browser.browser_manager import BrowserManager + + mgr = BrowserManager("lightpanda-test") + mgr.browser_type = "lightpanda" + + with ( + patch( + "code_puppy.tools.browser.browser_manager._load_plugin_browser_types" + ), + patch("code_puppy.tools.browser.browser_manager.emit_info"), + patch.object( + mgr, + "_initialize_lightpanda_browser", + new=AsyncMock(), + ) as mock_init_lightpanda, + ): + await mgr._initialize_browser() + mock_init_lightpanda.assert_called_once() + assert mgr._initialized is True + + @pytest.mark.asyncio + async def test_initialize_lightpanda_browser_sets_context(self): + from code_puppy.tools.browser.browser_manager import BrowserManager + + mgr = BrowserManager("lightpanda-init") + + mock_pw_instance = AsyncMock() + mock_browser = AsyncMock() + mock_context = AsyncMock() + mock_browser.contexts = [mock_context] + mock_pw_instance.chromium.connect_over_cdp.return_value = mock_browser + + mock_pw_class = AsyncMock() + mock_pw_class.start.return_value = mock_pw_instance + + mock_process = AsyncMock() + mock_process.returncode = None + mock_process.stderr = AsyncMock() + + with ( + patch("code_puppy.tools.browser.browser_manager.emit_info"), + patch( + "code_puppy.tools.browser.browser_manager.asyncio.create_subprocess_exec", + return_value=mock_process, + ), + patch("playwright.async_api.async_playwright", return_value=mock_pw_class), + patch.object(mgr, "_build_lightpanda_command", return_value=["lightpanda"]), + patch.object(mgr, "_get_lightpanda_host", return_value="127.0.0.1"), + patch.object(mgr, "_get_lightpanda_port", return_value=9222), + ): + await mgr._initialize_lightpanda_browser() + + assert mgr._playwright is mock_pw_instance + assert mgr._browser is mock_browser + assert mgr._context is mock_context + assert mgr._lightpanda_endpoint == "http://127.0.0.1:9222" + class TestCleanupSilent: @pytest.mark.asyncio @@ -186,6 +245,31 @@ async def test_cleanup_outer_exception(self): await mgr._cleanup(silent=False) assert mgr._initialized is False + @pytest.mark.asyncio + async def test_cleanup_stops_lightpanda_and_playwright(self): + from code_puppy.tools.browser.browser_manager import BrowserManager + + mgr = BrowserManager("lightpanda-cleanup") + mgr._initialized = True + mgr._context = AsyncMock() + mgr._context.storage_state = AsyncMock() + mgr._browser = AsyncMock() + mock_playwright = AsyncMock() + mgr._playwright = mock_playwright + mgr._lightpanda_process = AsyncMock() + + with ( + patch("code_puppy.tools.browser.browser_manager.emit_success"), + patch("code_puppy.tools.browser.browser_manager.emit_warning"), + patch.object( + mgr, "_stop_lightpanda_process", new=AsyncMock() + ) as mock_stop_lightpanda, + ): + await mgr._cleanup(silent=True) + + mock_playwright.stop.assert_called_once() + mock_stop_lightpanda.assert_called_once() + class TestSyncCleanup: def test_sync_cleanup_with_active_managers(self): From 69bcce6a705d8955d54d8e5ece111fc4117b83ef Mon Sep 17 00:00:00 2001 From: tdarei Date: Mon, 2 Mar 2026 20:55:39 +0000 Subject: [PATCH 05/11] Harden Lightpanda startup and validate browser catalog inputs --- code_puppy/mcp_/server_registry_catalog.py | 6 +- code_puppy/tools/browser/browser_manager.py | 124 ++++++++++++++---- tests/mcp/test_server_registry_catalog.py | 22 ++++ .../browser/test_browser_manager_full.py | 80 +++++++++++ 4 files changed, 205 insertions(+), 27 deletions(-) diff --git a/code_puppy/mcp_/server_registry_catalog.py b/code_puppy/mcp_/server_registry_catalog.py index 76a61dad3..31ee242b0 100644 --- a/code_puppy/mcp_/server_registry_catalog.py +++ b/code_puppy/mcp_/server_registry_catalog.py @@ -63,7 +63,11 @@ def get_environment_vars(self) -> List[str]: # Also check config for env vars (existing logic) if "env" in self.config: for _key, value in self.config["env"].items(): - if isinstance(value, str) and value.startswith("$"): + if ( + isinstance(value, str) + and value.startswith("$") + and not value.startswith("${") + ): var_name = value[1:] if var_name not in env_vars: env_vars.append(var_name) diff --git a/code_puppy/tools/browser/browser_manager.py b/code_puppy/tools/browser/browser_manager.py index f5828123a..1992265c6 100644 --- a/code_puppy/tools/browser/browser_manager.py +++ b/code_puppy/tools/browser/browser_manager.py @@ -22,6 +22,7 @@ # Registry for custom browser types from plugins (e.g., Camoufox for stealth browsing) _CUSTOM_BROWSER_TYPES: Dict[str, Callable] = {} _BROWSER_TYPES_LOADED: bool = False +_BUILTIN_PLAYWRIGHT_BROWSERS = {"chromium", "firefox", "webkit"} def _load_plugin_browser_types() -> None: @@ -181,18 +182,30 @@ def _get_lightpanda_host(self) -> str: """Get Lightpanda host.""" return os.getenv("LIGHTPANDA_HOST", "127.0.0.1") + @staticmethod + def _validate_tcp_port(port: int, source_name: str) -> int: + """Validate a TCP port number and return it.""" + if not 1 <= port <= 65535: + raise RuntimeError( + f"{source_name} must be between 1 and 65535, got: {port}" + ) + return port + def _get_lightpanda_port(self) -> int: """Get Lightpanda CDP port from env or an ephemeral free port.""" configured_port = os.getenv("LIGHTPANDA_PORT") if configured_port: try: - return int(configured_port) + parsed_port = int(configured_port) except ValueError as exc: raise RuntimeError( f"Invalid LIGHTPANDA_PORT value: {configured_port}" ) from exc + return self._validate_tcp_port(parsed_port, "LIGHTPANDA_PORT") - return self._find_free_port() + return self._validate_tcp_port( + self._find_free_port(), "auto-selected Lightpanda port" + ) @staticmethod def _get_lightpanda_startup_timeout() -> float: @@ -206,6 +219,18 @@ def _get_lightpanda_startup_timeout() -> float: ) from exc return max(timeout, 1.0) + @staticmethod + def _get_lightpanda_startup_retries() -> int: + """Get number of startup retries for auto-selected ports.""" + retries_raw = os.getenv("LIGHTPANDA_STARTUP_RETRIES", "3") + try: + retries = int(retries_raw) + except ValueError as exc: + raise RuntimeError( + f"Invalid LIGHTPANDA_STARTUP_RETRIES value: {retries_raw}" + ) from exc + return max(retries, 1) + async def _read_lightpanda_stderr(self) -> str: """Read Lightpanda stderr if available for better startup errors.""" if not self._lightpanda_process or not self._lightpanda_process.stderr: @@ -266,7 +291,11 @@ async def _connect_lightpanda_over_cdp(self, endpoint: str) -> Browser: ) async def _initialize_lightpanda_browser(self) -> None: - """Initialize Lightpanda and attach Playwright over CDP.""" + """Initialize Lightpanda and attach Playwright over CDP. + + Uses retry-on-failure for auto-selected ports to reduce startup + flakiness caused by bind races between probing and process launch. + """ from playwright.async_api import async_playwright if not self.headless: @@ -276,30 +305,57 @@ async def _initialize_lightpanda_browser(self) -> None: self.headless = True host = self._get_lightpanda_host() - port = self._get_lightpanda_port() - self._lightpanda_endpoint = f"http://{host}:{port}" + self._playwright = await async_playwright().start() + fixed_port = bool(os.getenv("LIGHTPANDA_PORT")) + max_attempts = 1 if fixed_port else self._get_lightpanda_startup_retries() + last_error: Optional[Exception] = None - command = self._build_lightpanda_command(host, port) - emit_info(f"Starting Lightpanda CDP endpoint at {host}:{port}") + for attempt in range(1, max_attempts + 1): + port = self._get_lightpanda_port() + self._lightpanda_endpoint = f"http://{host}:{port}" - self._lightpanda_process = await asyncio.create_subprocess_exec( - *command, - stdout=asyncio.subprocess.DEVNULL, - stderr=asyncio.subprocess.PIPE, - ) + command = self._build_lightpanda_command(host, port) + emit_info( + f"Starting Lightpanda CDP endpoint at {host}:{port} " + f"(attempt {attempt}/{max_attempts})" + ) - self._playwright = await async_playwright().start() - browser = await self._connect_lightpanda_over_cdp(self._lightpanda_endpoint) + self._lightpanda_process = await asyncio.create_subprocess_exec( + *command, + stdout=asyncio.subprocess.DEVNULL, + stderr=asyncio.subprocess.PIPE, + ) + + try: + browser = await self._connect_lightpanda_over_cdp( + self._lightpanda_endpoint + ) - # Reuse existing context when available (CDP default context), - # otherwise create one for consistent manager behavior. - if browser.contexts: - context = browser.contexts[0] - else: - context = await browser.new_context() + # Reuse existing context when available (CDP default context), + # otherwise create one for consistent manager behavior. + if browser.contexts: + context = browser.contexts[0] + else: + context = await browser.new_context() - self._browser = browser - self._context = context + self._browser = browser + self._context = context + return + except Exception as exc: + last_error = exc + await self._stop_lightpanda_process() + + if attempt < max_attempts: + emit_warning( + "Lightpanda startup failed; retrying with a new port " + f"(attempt {attempt + 1}/{max_attempts})." + ) + await asyncio.sleep(0.1) + + raise RuntimeError( + f"Failed to initialize Lightpanda after {max_attempts} attempts: " + f"{last_error}" + ) from last_error async def _stop_lightpanda_process(self) -> None: """Stop Lightpanda process if this manager started one.""" @@ -363,14 +419,30 @@ async def _initialize_browser(self) -> None: self._initialized = True return - # Default: use Playwright Chromium + requested_browser = (self.browser_type or "chromium").lower() + if requested_browser not in _BUILTIN_PLAYWRIGHT_BROWSERS: + supported_browsers = sorted( + _BUILTIN_PLAYWRIGHT_BROWSERS + | {"lightpanda"} + | set(_CUSTOM_BROWSER_TYPES.keys()) + ) + raise ValueError( + f"Unsupported browser_type '{self.browser_type}'. " + f"Supported values: {', '.join(supported_browsers)}" + ) + + # Default: use built-in Playwright browser backends from playwright.async_api import async_playwright - emit_info(f"Using persistent profile: {self.profile_dir}") + emit_info( + f"Using built-in Playwright browser '{requested_browser}' " + f"with persistent profile: {self.profile_dir}" + ) pw = await async_playwright().start() - # Use persistent context directory for Chromium to preserve browser state - context = await pw.chromium.launch_persistent_context( + self._playwright = pw + browser_launcher = getattr(pw, requested_browser) + context = await browser_launcher.launch_persistent_context( user_data_dir=str(self.profile_dir), headless=self.headless ) self._context = context diff --git a/tests/mcp/test_server_registry_catalog.py b/tests/mcp/test_server_registry_catalog.py index f19cd0a69..cfe5d353b 100644 --- a/tests/mcp/test_server_registry_catalog.py +++ b/tests/mcp/test_server_registry_catalog.py @@ -240,6 +240,28 @@ def test_get_environment_vars_mixed_sources(self): assert "MY_API_KEY" in env_vars assert len(env_vars) == 2 # No duplicates + def test_get_environment_vars_ignores_cmd_placeholders(self): + """Test env placeholder syntax `${...}` is not treated as env var input.""" + template = MCPServerTemplate( + id="test", + name="test", + display_name="Test", + description="Test", + category="Test", + tags=["test"], + type="stdio", + config={ + "env": { + "LIGHTPANDA_CDP_URL": "${cdp_url}", + "API_KEY": "$REAL_API_KEY", + } + }, + ) + + env_vars = template.get_environment_vars() + assert "REAL_API_KEY" in env_vars + assert "{cdp_url}" not in env_vars + def test_get_command_line_args(self): """Test getting command line arguments from requirements.""" args = [ diff --git a/tests/tools/browser/test_browser_manager_full.py b/tests/tools/browser/test_browser_manager_full.py index 667de4d74..2622aabf5 100644 --- a/tests/tools/browser/test_browser_manager_full.py +++ b/tests/tools/browser/test_browser_manager_full.py @@ -80,6 +80,7 @@ def test_get_session_browser_manager(self): class TestInitializeBrowser: @pytest.mark.asyncio async def test_initialize_browser_default_chromium(self): + """Test default browser initialization uses Chromium backend.""" from code_puppy.tools.browser.browser_manager import BrowserManager mgr = BrowserManager("init-test") @@ -106,6 +107,7 @@ async def test_initialize_browser_default_chromium(self): @pytest.mark.asyncio async def test_initialize_browser_custom_type(self): + """Test custom registered browser type initialization path.""" from code_puppy.tools.browser import browser_manager as bm from code_puppy.tools.browser.browser_manager import BrowserManager @@ -127,6 +129,7 @@ async def custom_init(manager): @pytest.mark.asyncio async def test_initialize_browser_lightpanda_type(self): + """Test `browser_type=lightpanda` dispatches to Lightpanda initializer.""" from code_puppy.tools.browser.browser_manager import BrowserManager mgr = BrowserManager("lightpanda-test") @@ -149,6 +152,7 @@ async def test_initialize_browser_lightpanda_type(self): @pytest.mark.asyncio async def test_initialize_lightpanda_browser_sets_context(self): + """Test Lightpanda init sets Playwright, browser, and context references.""" from code_puppy.tools.browser.browser_manager import BrowserManager mgr = BrowserManager("lightpanda-init") @@ -184,6 +188,81 @@ async def test_initialize_lightpanda_browser_sets_context(self): assert mgr._context is mock_context assert mgr._lightpanda_endpoint == "http://127.0.0.1:9222" + @pytest.mark.asyncio + async def test_initialize_lightpanda_retries_on_auto_port_failure(self): + """Test Lightpanda retries startup when an auto-port attempt fails.""" + from code_puppy.tools.browser.browser_manager import BrowserManager + + mgr = BrowserManager("lightpanda-retry") + + mock_pw_instance = AsyncMock() + mock_pw_class = AsyncMock() + mock_pw_class.start.return_value = mock_pw_instance + + mock_browser = AsyncMock() + mock_context = AsyncMock() + mock_browser.contexts = [mock_context] + + mock_process_a = AsyncMock() + mock_process_a.returncode = None + mock_process_a.stderr = AsyncMock() + mock_process_b = AsyncMock() + mock_process_b.returncode = None + mock_process_b.stderr = AsyncMock() + + with ( + patch("code_puppy.tools.browser.browser_manager.emit_info"), + patch("code_puppy.tools.browser.browser_manager.emit_warning"), + patch("playwright.async_api.async_playwright", return_value=mock_pw_class), + patch( + "code_puppy.tools.browser.browser_manager.asyncio.create_subprocess_exec", + side_effect=[mock_process_a, mock_process_b], + ), + patch.object(mgr, "_get_lightpanda_host", return_value="127.0.0.1"), + patch.object(mgr, "_get_lightpanda_port", side_effect=[9222, 9223]), + patch.object(mgr, "_get_lightpanda_startup_retries", return_value=2), + patch.object(mgr, "_build_lightpanda_command", return_value=["lightpanda"]), + patch.object( + mgr, + "_connect_lightpanda_over_cdp", + side_effect=[RuntimeError("bind fail"), mock_browser], + ), + patch.object(mgr, "_stop_lightpanda_process", new=AsyncMock()) as mock_stop, + patch.dict("os.environ", {"LIGHTPANDA_PORT": ""}, clear=False), + ): + await mgr._initialize_lightpanda_browser() + + assert mock_stop.await_count == 1 + assert mgr._context is mock_context + + @pytest.mark.asyncio + async def test_initialize_browser_unknown_type_raises(self): + """Test unsupported built-in browser types fail fast with clear error.""" + from code_puppy.tools.browser.browser_manager import BrowserManager + + mgr = BrowserManager("bad-browser-type") + mgr.browser_type = "unknown-browser" + + with ( + patch( + "code_puppy.tools.browser.browser_manager._load_plugin_browser_types" + ), + pytest.raises(ValueError, match="Unsupported browser_type"), + ): + await mgr._initialize_browser() + + def test_get_lightpanda_port_rejects_invalid_range(self): + """Test LIGHTPANDA_PORT values outside valid TCP range are rejected.""" + from code_puppy.tools.browser.browser_manager import BrowserManager + + mgr = BrowserManager("bad-port") + + with patch.dict("os.environ", {"LIGHTPANDA_PORT": "70000"}, clear=False): + with pytest.raises( + RuntimeError, match="LIGHTPANDA_PORT must be between 1 and 65535" + ): + mgr._get_lightpanda_port() + class TestCleanupSilent: @pytest.mark.asyncio @@ -247,6 +326,7 @@ async def test_cleanup_outer_exception(self): @pytest.mark.asyncio async def test_cleanup_stops_lightpanda_and_playwright(self): + """Test cleanup stops both Playwright and Lightpanda process.""" from code_puppy.tools.browser.browser_manager import BrowserManager mgr = BrowserManager("lightpanda-cleanup") From 0fed5a5c3d969396884795f116ec85b1b5cf9cb6 Mon Sep 17 00:00:00 2001 From: tdarei Date: Mon, 2 Mar 2026 21:03:52 +0000 Subject: [PATCH 06/11] Drain Lightpanda stderr during startup and harden placeholder test --- code_puppy/tools/browser/browser_manager.py | 59 ++++++++++++++++++--- tests/mcp/test_server_registry_catalog.py | 1 + 2 files changed, 54 insertions(+), 6 deletions(-) diff --git a/code_puppy/tools/browser/browser_manager.py b/code_puppy/tools/browser/browser_manager.py index 1992265c6..7ddfbf4f0 100644 --- a/code_puppy/tools/browser/browser_manager.py +++ b/code_puppy/tools/browser/browser_manager.py @@ -11,6 +11,7 @@ import shlex import shutil import socket +from collections import deque from pathlib import Path from typing import Callable, Dict, Optional @@ -114,6 +115,7 @@ class BrowserManager: _playwright: Optional[object] = None _lightpanda_process: Optional[asyncio.subprocess.Process] = None _lightpanda_endpoint: Optional[str] = None + _lightpanda_stderr_task: Optional[asyncio.Task] = None _initialized: bool = False def __init__( @@ -137,6 +139,7 @@ def __init__( # Unique profile directory per session for browser state self.profile_dir = self._get_profile_directory() + self._lightpanda_stderr_buffer: deque[str] = deque(maxlen=128) def _get_profile_directory(self) -> Path: """Get or create the profile directory for this session. @@ -233,15 +236,55 @@ def _get_lightpanda_startup_retries() -> int: async def _read_lightpanda_stderr(self) -> str: """Read Lightpanda stderr if available for better startup errors.""" - if not self._lightpanda_process or not self._lightpanda_process.stderr: + if not self._lightpanda_stderr_buffer: return "" + return "\n".join(self._lightpanda_stderr_buffer).strip()[:500] - with contextlib.suppress(Exception): - data = await self._lightpanda_process.stderr.read() - if data: - return data.decode(errors="replace").strip()[:500] + async def _drain_lightpanda_stderr( + self, stream: asyncio.StreamReader + ) -> None: + """Drain stderr continuously to avoid subprocess pipe backpressure.""" + try: + while True: + line = await stream.readline() + if not line: + return + decoded = line.decode(errors="replace").strip() + if decoded: + self._lightpanda_stderr_buffer.append(decoded) + except asyncio.CancelledError: + raise + except Exception: + # Stderr draining is best effort and should not fail startup. + return + + def _start_lightpanda_stderr_drain(self) -> None: + """Start background stderr drain task when stream is available.""" + stream = self._lightpanda_process.stderr if self._lightpanda_process else None + + if isinstance(stream, asyncio.StreamReader): + self._lightpanda_stderr_task = asyncio.create_task( + self._drain_lightpanda_stderr(stream) + ) + else: + self._lightpanda_stderr_task = None - return "" + async def _stop_lightpanda_stderr_drain(self) -> None: + """Stop background stderr drain task.""" + task = self._lightpanda_stderr_task + self._lightpanda_stderr_task = None + + if not task: + return + + if task.done(): + with contextlib.suppress(Exception): + await task + return + + task.cancel() + with contextlib.suppress(asyncio.CancelledError, Exception): + await task def _build_lightpanda_command(self, host: str, port: int) -> list[str]: """Build the Lightpanda startup command.""" @@ -325,6 +368,8 @@ async def _initialize_lightpanda_browser(self) -> None: stdout=asyncio.subprocess.DEVNULL, stderr=asyncio.subprocess.PIPE, ) + self._lightpanda_stderr_buffer.clear() + self._start_lightpanda_stderr_drain() try: browser = await self._connect_lightpanda_over_cdp( @@ -375,6 +420,8 @@ async def _stop_lightpanda_process(self) -> None: with contextlib.suppress(Exception): await process.wait() + await self._stop_lightpanda_stderr_drain() + async def async_initialize(self) -> None: """Initialize a browser backend.""" if self._initialized: diff --git a/tests/mcp/test_server_registry_catalog.py b/tests/mcp/test_server_registry_catalog.py index cfe5d353b..67a813d22 100644 --- a/tests/mcp/test_server_registry_catalog.py +++ b/tests/mcp/test_server_registry_catalog.py @@ -261,6 +261,7 @@ def test_get_environment_vars_ignores_cmd_placeholders(self): env_vars = template.get_environment_vars() assert "REAL_API_KEY" in env_vars assert "{cdp_url}" not in env_vars + assert "cdp_url" not in env_vars def test_get_command_line_args(self): """Test getting command line arguments from requirements.""" From 3ede30844340be3aa0b470e3720b8fc5f9053d55 Mon Sep 17 00:00:00 2001 From: tdarei Date: Mon, 2 Mar 2026 21:07:53 +0000 Subject: [PATCH 07/11] Further harden Lightpanda stderr draining and teardown --- code_puppy/tools/browser/browser_manager.py | 37 ++++++++++++--------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/code_puppy/tools/browser/browser_manager.py b/code_puppy/tools/browser/browser_manager.py index 7ddfbf4f0..d30cc83c3 100644 --- a/code_puppy/tools/browser/browser_manager.py +++ b/code_puppy/tools/browser/browser_manager.py @@ -238,7 +238,8 @@ async def _read_lightpanda_stderr(self) -> str: """Read Lightpanda stderr if available for better startup errors.""" if not self._lightpanda_stderr_buffer: return "" - return "\n".join(self._lightpanda_stderr_buffer).strip()[:500] + stderr_text = "\n".join(self._lightpanda_stderr_buffer).strip() + return stderr_text[-500:] async def _drain_lightpanda_stderr( self, stream: asyncio.StreamReader @@ -246,12 +247,14 @@ async def _drain_lightpanda_stderr( """Drain stderr continuously to avoid subprocess pipe backpressure.""" try: while True: - line = await stream.readline() - if not line: + chunk = await stream.read(4096) + if not chunk: return - decoded = line.decode(errors="replace").strip() - if decoded: - self._lightpanda_stderr_buffer.append(decoded) + decoded = chunk.decode(errors="replace") + for line in decoded.splitlines(): + stripped = line.strip() + if stripped: + self._lightpanda_stderr_buffer.append(stripped) except asyncio.CancelledError: raise except Exception: @@ -260,6 +263,9 @@ async def _drain_lightpanda_stderr( def _start_lightpanda_stderr_drain(self) -> None: """Start background stderr drain task when stream is available.""" + if self._lightpanda_stderr_task and not self._lightpanda_stderr_task.done(): + self._lightpanda_stderr_task.cancel() + stream = self._lightpanda_process.stderr if self._lightpanda_process else None if isinstance(stream, asyncio.StreamReader): @@ -404,19 +410,20 @@ async def _initialize_lightpanda_browser(self) -> None: async def _stop_lightpanda_process(self) -> None: """Stop Lightpanda process if this manager started one.""" - if not self._lightpanda_process: - return - process = self._lightpanda_process self._lightpanda_process = None self._lightpanda_endpoint = None - if process.returncode is None: - process.terminate() - try: - await asyncio.wait_for(process.wait(), timeout=3) - except asyncio.TimeoutError: - process.kill() + if process: + if process.returncode is None: + process.terminate() + try: + await asyncio.wait_for(process.wait(), timeout=3) + except asyncio.TimeoutError: + process.kill() + with contextlib.suppress(Exception): + await process.wait() + else: with contextlib.suppress(Exception): await process.wait() From 6383e4bb722d58c1ee475d89a13f264d277b8f9c Mon Sep 17 00:00:00 2001 From: tdarei Date: Mon, 2 Mar 2026 21:45:34 +0000 Subject: [PATCH 08/11] Harden Lightpanda executable/timeout handling and add regressions --- code_puppy/tools/browser/browser_manager.py | 36 +++++---- .../browser/test_browser_manager_full.py | 74 +++++++++++++++++++ 2 files changed, 97 insertions(+), 13 deletions(-) diff --git a/code_puppy/tools/browser/browser_manager.py b/code_puppy/tools/browser/browser_manager.py index d30cc83c3..8504ddf35 100644 --- a/code_puppy/tools/browser/browser_manager.py +++ b/code_puppy/tools/browser/browser_manager.py @@ -171,14 +171,17 @@ def _resolve_lightpanda_executable(self) -> str: ) if path_like: - if Path(executable).exists(): - return executable - elif shutil.which(executable): - return executable + executable_path = Path(executable).expanduser() + if executable_path.is_file() and os.access(executable_path, os.X_OK): + return str(executable_path) + else: + resolved = shutil.which(executable) + if resolved: + return resolved raise RuntimeError( - "Lightpanda executable not found. Install Lightpanda or set " - "LIGHTPANDA_EXECUTABLE to a valid path." + "Lightpanda executable not found or not executable. Install Lightpanda " + "or set LIGHTPANDA_EXECUTABLE to a valid executable path." ) def _get_lightpanda_host(self) -> str: @@ -241,9 +244,7 @@ async def _read_lightpanda_stderr(self) -> str: stderr_text = "\n".join(self._lightpanda_stderr_buffer).strip() return stderr_text[-500:] - async def _drain_lightpanda_stderr( - self, stream: asyncio.StreamReader - ) -> None: + async def _drain_lightpanda_stderr(self, stream: asyncio.StreamReader) -> None: """Drain stderr continuously to avoid subprocess pipe backpressure.""" try: while True: @@ -330,10 +331,19 @@ async def _connect_lightpanda_over_cdp(self, endpoint: str) -> Browser: try: if not self._playwright: raise RuntimeError("Playwright is not initialized.") - return await self._playwright.chromium.connect_over_cdp(endpoint) + remaining = deadline - loop.time() + if remaining <= 0: + break + return await asyncio.wait_for( + self._playwright.chromium.connect_over_cdp(endpoint), + timeout=remaining, + ) except Exception as exc: last_error = exc - await asyncio.sleep(0.2) + remaining = deadline - loop.time() + if remaining <= 0: + break + await asyncio.sleep(min(0.2, remaining)) raise RuntimeError( f"Timed out connecting to Lightpanda CDP at {endpoint}: {last_error}" @@ -454,6 +464,7 @@ async def _initialize_browser(self) -> None: """ # Load plugin browser types on first initialization _load_plugin_browser_types() + requested_browser = (self.browser_type or "chromium").lower() # Check if a custom browser type was requested and is available if self.browser_type and self.browser_type in _CUSTOM_BROWSER_TYPES: @@ -467,13 +478,12 @@ async def _initialize_browser(self) -> None: self._initialized = True return - if self.browser_type == "lightpanda": + if requested_browser == "lightpanda": emit_info(f"Using Lightpanda browser (session: {self.session_id})") await self._initialize_lightpanda_browser() self._initialized = True return - requested_browser = (self.browser_type or "chromium").lower() if requested_browser not in _BUILTIN_PLAYWRIGHT_BROWSERS: supported_browsers = sorted( _BUILTIN_PLAYWRIGHT_BROWSERS diff --git a/tests/tools/browser/test_browser_manager_full.py b/tests/tools/browser/test_browser_manager_full.py index 2622aabf5..ead5fc8e2 100644 --- a/tests/tools/browser/test_browser_manager_full.py +++ b/tests/tools/browser/test_browser_manager_full.py @@ -150,6 +150,29 @@ async def test_initialize_browser_lightpanda_type(self): mock_init_lightpanda.assert_called_once() assert mgr._initialized is True + @pytest.mark.asyncio + async def test_initialize_browser_lightpanda_type_case_insensitive(self): + """Test mixed-case lightpanda browser type dispatches correctly.""" + from code_puppy.tools.browser.browser_manager import BrowserManager + + mgr = BrowserManager("lightpanda-case-test") + mgr.browser_type = "LightPanda" + + with ( + patch( + "code_puppy.tools.browser.browser_manager._load_plugin_browser_types" + ), + patch("code_puppy.tools.browser.browser_manager.emit_info"), + patch.object( + mgr, + "_initialize_lightpanda_browser", + new=AsyncMock(), + ) as mock_init_lightpanda, + ): + await mgr._initialize_browser() + mock_init_lightpanda.assert_called_once() + assert mgr._initialized is True + @pytest.mark.asyncio async def test_initialize_lightpanda_browser_sets_context(self): """Test Lightpanda init sets Playwright, browser, and context references.""" @@ -235,6 +258,29 @@ async def test_initialize_lightpanda_retries_on_auto_port_failure(self): assert mock_stop.await_count == 1 assert mgr._context is mock_context + @pytest.mark.asyncio + async def test_connect_lightpanda_over_cdp_attempt_respects_timeout_budget(self): + """Test CDP connect attempts are bounded by LIGHTPANDA_STARTUP_TIMEOUT.""" + import asyncio + + from code_puppy.tools.browser.browser_manager import BrowserManager + + mgr = BrowserManager("lightpanda-cdp-timeout") + mock_playwright = MagicMock() + mock_playwright.chromium = MagicMock() + + async def slow_connect(_endpoint): + await asyncio.sleep(5) + + mock_playwright.chromium.connect_over_cdp = AsyncMock(side_effect=slow_connect) + mgr._playwright = mock_playwright + + with patch.object(mgr, "_get_lightpanda_startup_timeout", return_value=0.1): + with pytest.raises( + RuntimeError, match="Timed out connecting to Lightpanda" + ): + await mgr._connect_lightpanda_over_cdp("http://127.0.0.1:9222") + @pytest.mark.asyncio async def test_initialize_browser_unknown_type_raises(self): """Test unsupported built-in browser types fail fast with clear error.""" @@ -251,6 +297,34 @@ async def test_initialize_browser_unknown_type_raises(self): ): await mgr._initialize_browser() + def test_resolve_lightpanda_executable_rejects_directory_path(self, tmp_path): + """Test path-like LIGHTPANDA_EXECUTABLE must be an executable file.""" + from code_puppy.tools.browser.browser_manager import BrowserManager + + mgr = BrowserManager("bad-executable") + + with patch.dict( + "os.environ", {"LIGHTPANDA_EXECUTABLE": str(tmp_path)}, clear=False + ): + with pytest.raises(RuntimeError, match="not found or not executable"): + mgr._resolve_lightpanda_executable() + + def test_resolve_lightpanda_executable_accepts_pathlike_executable(self): + """Test path-like LIGHTPANDA_EXECUTABLE resolves a valid executable file.""" + import sys + from pathlib import Path + + from code_puppy.tools.browser.browser_manager import BrowserManager + + mgr = BrowserManager("good-executable") + + with patch.dict( + "os.environ", {"LIGHTPANDA_EXECUTABLE": sys.executable}, clear=False + ): + resolved = mgr._resolve_lightpanda_executable() + + assert Path(resolved).resolve() == Path(sys.executable).resolve() + def test_get_lightpanda_port_rejects_invalid_range(self): """Test LIGHTPANDA_PORT values outside valid TCP range are rejected.""" from code_puppy.tools.browser.browser_manager import BrowserManager From 660b1bcef0b6dcec98bfed496d81ff81e16f6013 Mon Sep 17 00:00:00 2001 From: tdarei Date: Mon, 2 Mar 2026 22:45:18 +0000 Subject: [PATCH 09/11] Validate Lightpanda startup timeout and tighten async assertions --- code_puppy/tools/browser/browser_manager.py | 7 +++++++ .../browser/test_browser_manager_full.py | 20 +++++++++++++++++-- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/code_puppy/tools/browser/browser_manager.py b/code_puppy/tools/browser/browser_manager.py index 8504ddf35..679d6b4d6 100644 --- a/code_puppy/tools/browser/browser_manager.py +++ b/code_puppy/tools/browser/browser_manager.py @@ -7,6 +7,7 @@ import atexit import contextlib import contextvars +import math import os import shlex import shutil @@ -223,6 +224,12 @@ def _get_lightpanda_startup_timeout() -> float: raise RuntimeError( f"Invalid LIGHTPANDA_STARTUP_TIMEOUT value: {timeout_raw}" ) from exc + + if not math.isfinite(timeout) or timeout <= 0: + raise RuntimeError( + "LIGHTPANDA_STARTUP_TIMEOUT must be a finite positive number, " + f"got: {timeout_raw}" + ) return max(timeout, 1.0) @staticmethod diff --git a/tests/tools/browser/test_browser_manager_full.py b/tests/tools/browser/test_browser_manager_full.py index ead5fc8e2..f12a6bba4 100644 --- a/tests/tools/browser/test_browser_manager_full.py +++ b/tests/tools/browser/test_browser_manager_full.py @@ -337,6 +337,22 @@ def test_get_lightpanda_port_rejects_invalid_range(self): ): mgr._get_lightpanda_port() + @pytest.mark.parametrize("raw_timeout", ["nan", "inf", "-1", "0"]) + def test_get_lightpanda_startup_timeout_rejects_non_finite_or_non_positive( + self, raw_timeout + ): + """Test invalid LIGHTPANDA_STARTUP_TIMEOUT values fail with clear errors.""" + from code_puppy.tools.browser.browser_manager import BrowserManager + + with patch.dict( + "os.environ", {"LIGHTPANDA_STARTUP_TIMEOUT": raw_timeout}, clear=False + ): + with pytest.raises( + RuntimeError, + match="LIGHTPANDA_STARTUP_TIMEOUT must be a finite positive number", + ): + BrowserManager._get_lightpanda_startup_timeout() + class TestCleanupSilent: @pytest.mark.asyncio @@ -421,8 +437,8 @@ async def test_cleanup_stops_lightpanda_and_playwright(self): ): await mgr._cleanup(silent=True) - mock_playwright.stop.assert_called_once() - mock_stop_lightpanda.assert_called_once() + mock_playwright.stop.assert_awaited_once() + mock_stop_lightpanda.assert_awaited_once() class TestSyncCleanup: From ea79c33278b9db9cdcb5ef49de5b32213af9dc6a Mon Sep 17 00:00:00 2001 From: tdarei Date: Mon, 2 Mar 2026 22:54:08 +0000 Subject: [PATCH 10/11] Guarantee Lightpanda stderr drain cleanup and await async init mocks --- code_puppy/tools/browser/browser_manager.py | 18 ++++++++++---- .../browser/test_browser_manager_full.py | 24 +++++++++++++++++-- 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/code_puppy/tools/browser/browser_manager.py b/code_puppy/tools/browser/browser_manager.py index 679d6b4d6..64f7281b9 100644 --- a/code_puppy/tools/browser/browser_manager.py +++ b/code_puppy/tools/browser/browser_manager.py @@ -431,20 +431,28 @@ async def _stop_lightpanda_process(self) -> None: self._lightpanda_process = None self._lightpanda_endpoint = None - if process: + try: + if not process: + return + if process.returncode is None: - process.terminate() + with contextlib.suppress(Exception): + process.terminate() try: await asyncio.wait_for(process.wait(), timeout=3) except asyncio.TimeoutError: - process.kill() + with contextlib.suppress(Exception): + process.kill() + with contextlib.suppress(Exception): + await process.wait() + except Exception: with contextlib.suppress(Exception): await process.wait() else: with contextlib.suppress(Exception): await process.wait() - - await self._stop_lightpanda_stderr_drain() + finally: + await self._stop_lightpanda_stderr_drain() async def async_initialize(self) -> None: """Initialize a browser backend.""" diff --git a/tests/tools/browser/test_browser_manager_full.py b/tests/tools/browser/test_browser_manager_full.py index f12a6bba4..b77dca9f8 100644 --- a/tests/tools/browser/test_browser_manager_full.py +++ b/tests/tools/browser/test_browser_manager_full.py @@ -147,7 +147,7 @@ async def test_initialize_browser_lightpanda_type(self): ) as mock_init_lightpanda, ): await mgr._initialize_browser() - mock_init_lightpanda.assert_called_once() + mock_init_lightpanda.assert_awaited_once() assert mgr._initialized is True @pytest.mark.asyncio @@ -170,7 +170,7 @@ async def test_initialize_browser_lightpanda_type_case_insensitive(self): ) as mock_init_lightpanda, ): await mgr._initialize_browser() - mock_init_lightpanda.assert_called_once() + mock_init_lightpanda.assert_awaited_once() assert mgr._initialized is True @pytest.mark.asyncio @@ -440,6 +440,26 @@ async def test_cleanup_stops_lightpanda_and_playwright(self): mock_playwright.stop.assert_awaited_once() mock_stop_lightpanda.assert_awaited_once() + @pytest.mark.asyncio + async def test_stop_lightpanda_process_always_stops_stderr_drain(self): + """Test stderr drain cleanup still runs when process termination fails.""" + from code_puppy.tools.browser.browser_manager import BrowserManager + + mgr = BrowserManager("lightpanda-stop-failure") + + mock_process = MagicMock() + mock_process.returncode = None + mock_process.terminate.side_effect = ProcessLookupError("already exited") + mock_process.wait = AsyncMock(return_value=None) + mgr._lightpanda_process = mock_process + + with patch.object( + mgr, "_stop_lightpanda_stderr_drain", new=AsyncMock() + ) as mock_stop_stderr: + await mgr._stop_lightpanda_process() + + mock_stop_stderr.assert_awaited_once() + class TestSyncCleanup: def test_sync_cleanup_with_active_managers(self): From a8f5cbe50bfda37faaf92e310d0440a126357f68 Mon Sep 17 00:00:00 2001 From: tdarei Date: Mon, 2 Mar 2026 23:07:02 +0000 Subject: [PATCH 11/11] Normalize custom browser type matching and exercise stderr drain path --- code_puppy/tools/browser/browser_manager.py | 29 ++++++++++++-- .../browser/test_browser_manager_full.py | 40 +++++++++++++++++-- 2 files changed, 62 insertions(+), 7 deletions(-) diff --git a/code_puppy/tools/browser/browser_manager.py b/code_puppy/tools/browser/browser_manager.py index 64f7281b9..2d8369216 100644 --- a/code_puppy/tools/browser/browser_manager.py +++ b/code_puppy/tools/browser/browser_manager.py @@ -481,13 +481,34 @@ async def _initialize_browser(self) -> None: _load_plugin_browser_types() requested_browser = (self.browser_type or "chromium").lower() - # Check if a custom browser type was requested and is available - if self.browser_type and self.browser_type in _CUSTOM_BROWSER_TYPES: + # Check if a custom browser type was requested and is available. + # Match exact key first, then case-insensitive for consistency with + # built-in/lightpanda browser routing. + custom_browser_key: Optional[str] = None + if self.browser_type: + if self.browser_type in _CUSTOM_BROWSER_TYPES: + custom_browser_key = self.browser_type + else: + matched_custom_keys = [ + key + for key in _CUSTOM_BROWSER_TYPES + if key.lower() == requested_browser + ] + if len(matched_custom_keys) == 1: + custom_browser_key = matched_custom_keys[0] + elif len(matched_custom_keys) > 1: + raise ValueError( + f"Ambiguous custom browser_type '{self.browser_type}'. " + "Multiple plugin browser types match case-insensitively: " + f"{', '.join(sorted(matched_custom_keys))}" + ) + + if custom_browser_key: emit_info( - f"Using custom browser type '{self.browser_type}' " + f"Using custom browser type '{custom_browser_key}' " f"(session: {self.session_id})" ) - init_func = _CUSTOM_BROWSER_TYPES[self.browser_type] + init_func = _CUSTOM_BROWSER_TYPES[custom_browser_key] # Custom init functions should set self._context and self._browser await init_func(self) self._initialized = True diff --git a/tests/tools/browser/test_browser_manager_full.py b/tests/tools/browser/test_browser_manager_full.py index b77dca9f8..bfd6c39e5 100644 --- a/tests/tools/browser/test_browser_manager_full.py +++ b/tests/tools/browser/test_browser_manager_full.py @@ -1,5 +1,6 @@ """Full coverage tests for browser_manager.py.""" +import asyncio from unittest.mock import AsyncMock, MagicMock, patch import pytest @@ -127,6 +128,28 @@ async def custom_init(manager): del bm._CUSTOM_BROWSER_TYPES["custom"] + @pytest.mark.asyncio + async def test_initialize_browser_custom_type_case_insensitive(self): + """Test mixed-case custom browser type resolves case-insensitively.""" + from code_puppy.tools.browser import browser_manager as bm + from code_puppy.tools.browser.browser_manager import BrowserManager + + async def custom_init(manager): + manager._context = AsyncMock() + manager._browser = AsyncMock() + + bm._CUSTOM_BROWSER_TYPES["myplugin"] = custom_init + bm._BROWSER_TYPES_LOADED = True + + mgr = BrowserManager("custom-case-test") + mgr.browser_type = "MyPlugin" + + with patch("code_puppy.tools.browser.browser_manager.emit_info"): + await mgr._initialize_browser() + assert mgr._initialized is True + + del bm._CUSTOM_BROWSER_TYPES["myplugin"] + @pytest.mark.asyncio async def test_initialize_browser_lightpanda_type(self): """Test `browser_type=lightpanda` dispatches to Lightpanda initializer.""" @@ -191,7 +214,10 @@ async def test_initialize_lightpanda_browser_sets_context(self): mock_process = AsyncMock() mock_process.returncode = None - mock_process.stderr = AsyncMock() + stream = asyncio.StreamReader() + stream.feed_data(b"lightpanda started\n") + stream.feed_eof() + mock_process.stderr = stream with ( patch("code_puppy.tools.browser.browser_manager.emit_info"), @@ -205,11 +231,13 @@ async def test_initialize_lightpanda_browser_sets_context(self): patch.object(mgr, "_get_lightpanda_port", return_value=9222), ): await mgr._initialize_lightpanda_browser() + await asyncio.sleep(0) assert mgr._playwright is mock_pw_instance assert mgr._browser is mock_browser assert mgr._context is mock_context assert mgr._lightpanda_endpoint == "http://127.0.0.1:9222" + assert mgr._lightpanda_stderr_task is not None @pytest.mark.asyncio async def test_initialize_lightpanda_retries_on_auto_port_failure(self): @@ -228,10 +256,16 @@ async def test_initialize_lightpanda_retries_on_auto_port_failure(self): mock_process_a = AsyncMock() mock_process_a.returncode = None - mock_process_a.stderr = AsyncMock() + stream_a = asyncio.StreamReader() + stream_a.feed_data(b"bind failed\n") + stream_a.feed_eof() + mock_process_a.stderr = stream_a mock_process_b = AsyncMock() mock_process_b.returncode = None - mock_process_b.stderr = AsyncMock() + stream_b = asyncio.StreamReader() + stream_b.feed_data(b"retrying\n") + stream_b.feed_eof() + mock_process_b.stderr = stream_b with ( patch("code_puppy.tools.browser.browser_manager.emit_info"),