From 0bb7ed1d9594bd2939afea6e64928ad790b69fa1 Mon Sep 17 00:00:00 2001 From: kshitij <82637225+kshitijk4poor@users.noreply.github.com> Date: Sat, 14 Mar 2026 02:56:06 -0700 Subject: [PATCH] refactor: salvage adapter and CLI cleanup from PR #939 Salvaged from PR #939 by kshitij. - deduplicate Discord slash command dispatch and local file send helpers - deduplicate Slack file uploads while preserving thread metadata - extract shared CLI session relative-time formatting - hoist browser PATH cleanup constants and throttle screenshot pruning - tidy small type and import cleanups --- agent/display.py | 2 +- gateway/platforms/discord.py | 244 +++++++------------------- gateway/platforms/slack.py | 58 +++--- hermes_cli/main.py | 85 +++------ hermes_state.py | 3 - tests/gateway/test_send_image_file.py | 34 +++- tools/browser_tool.py | 20 ++- 7 files changed, 164 insertions(+), 282 deletions(-) diff --git a/agent/display.py b/agent/display.py index 88926cd94..72b56318d 100644 --- a/agent/display.py +++ b/agent/display.py @@ -68,7 +68,7 @@ def _oneline(text: str) -> str: return " ".join(text.split()) -def build_tool_preview(tool_name: str, args: dict, max_len: int = 40) -> str: +def build_tool_preview(tool_name: str, args: dict, max_len: int = 40) -> str | None: """Build a short preview of a tool call's primary argument for display.""" if not args: return None diff --git a/gateway/platforms/discord.py b/gateway/platforms/discord.py index e05a421e1..47760d236 100644 --- a/gateway/platforms/discord.py +++ b/gateway/platforms/discord.py @@ -265,6 +265,28 @@ class DiscordAdapter(BasePlatformAdapter): logger.error("[%s] Failed to edit Discord message %s: %s", self.name, message_id, e, exc_info=True) return SendResult(success=False, error=str(e)) + async def _send_file_attachment( + self, + chat_id: str, + file_path: str, + caption: Optional[str] = None, + ) -> SendResult: + """Send a local file as a Discord attachment.""" + if not self._client: + return SendResult(success=False, error="Not connected") + + channel = self._client.get_channel(int(chat_id)) + if not channel: + channel = await self._client.fetch_channel(int(chat_id)) + if not channel: + return SendResult(success=False, error=f"Channel {chat_id} not found") + + filename = os.path.basename(file_path) + with open(file_path, "rb") as fh: + file = discord.File(fh, filename=filename) + msg = await channel.send(content=caption if caption else None, file=file) + return SendResult(success=True, message_id=str(msg.id)) + async def send_voice( self, chat_id: str, @@ -274,36 +296,14 @@ class DiscordAdapter(BasePlatformAdapter): metadata: Optional[Dict[str, Any]] = None, ) -> SendResult: """Send audio as a Discord file attachment.""" - if not self._client: - return SendResult(success=False, error="Not connected") - try: - import io - - channel = self._client.get_channel(int(chat_id)) - if not channel: - channel = await self._client.fetch_channel(int(chat_id)) - if not channel: - return SendResult(success=False, error=f"Channel {chat_id} not found") - - if not os.path.exists(audio_path): - return SendResult(success=False, error=f"Audio file not found: {audio_path}") - - # Determine filename from path - filename = os.path.basename(audio_path) - - with open(audio_path, "rb") as f: - file = discord.File(io.BytesIO(f.read()), filename=filename) - msg = await channel.send( - content=caption if caption else None, - file=file, - ) - return SendResult(success=True, message_id=str(msg.id)) - + return await self._send_file_attachment(chat_id, audio_path, caption) + except FileNotFoundError: + return SendResult(success=False, error=f"Audio file not found: {audio_path}") except Exception as e: # pragma: no cover - defensive logging logger.error("[%s] Failed to send audio, falling back to base adapter: %s", self.name, e, exc_info=True) - return await super().send_voice(chat_id, audio_path, caption, reply_to) - + return await super().send_voice(chat_id, audio_path, caption, reply_to, metadata=metadata) + async def send_image_file( self, chat_id: str, @@ -313,34 +313,13 @@ class DiscordAdapter(BasePlatformAdapter): metadata: Optional[Dict[str, Any]] = None, ) -> SendResult: """Send a local image file natively as a Discord file attachment.""" - if not self._client: - return SendResult(success=False, error="Not connected") - try: - import io - - channel = self._client.get_channel(int(chat_id)) - if not channel: - channel = await self._client.fetch_channel(int(chat_id)) - if not channel: - return SendResult(success=False, error=f"Channel {chat_id} not found") - - if not os.path.exists(image_path): - return SendResult(success=False, error=f"Image file not found: {image_path}") - - filename = os.path.basename(image_path) - - with open(image_path, "rb") as f: - file = discord.File(io.BytesIO(f.read()), filename=filename) - msg = await channel.send( - content=caption if caption else None, - file=file, - ) - return SendResult(success=True, message_id=str(msg.id)) - + return await self._send_file_attachment(chat_id, image_path, caption) + except FileNotFoundError: + return SendResult(success=False, error=f"Image file not found: {image_path}") except Exception as e: # pragma: no cover - defensive logging logger.error("[%s] Failed to send local image, falling back to base adapter: %s", self.name, e, exc_info=True) - return await super().send_image_file(chat_id, image_path, caption, reply_to) + return await super().send_image_file(chat_id, image_path, caption, reply_to, metadata=metadata) async def send_image( self, @@ -528,7 +507,22 @@ class DiscordAdapter(BasePlatformAdapter): """ # Discord markdown is fairly standard, no special escaping needed return content - + + async def _run_simple_slash( + self, + interaction: discord.Interaction, + command_text: str, + followup_msg: str = "Done~", + ) -> None: + """Common handler for simple slash commands that dispatch a command string.""" + await interaction.response.defer(ephemeral=True) + event = self._build_slash_event(interaction, command_text) + await self.handle_message(event) + try: + await interaction.followup.send(followup_msg, ephemeral=True) + except Exception as e: + logger.debug("Discord followup failed: %s", e) + def _register_slash_commands(self) -> None: """Register Discord slash commands on the command tree.""" if not self._client: @@ -551,34 +545,16 @@ class DiscordAdapter(BasePlatformAdapter): @tree.command(name="new", description="Start a new conversation") async def slash_new(interaction: discord.Interaction): - await interaction.response.defer(ephemeral=True) - event = self._build_slash_event(interaction, "/reset") - await self.handle_message(event) - try: - await interaction.followup.send("New conversation started~", ephemeral=True) - except Exception as e: - logger.debug("Discord followup failed: %s", e) + await self._run_simple_slash(interaction, "/reset", "New conversation started~") @tree.command(name="reset", description="Reset your Hermes session") async def slash_reset(interaction: discord.Interaction): - await interaction.response.defer(ephemeral=True) - event = self._build_slash_event(interaction, "/reset") - await self.handle_message(event) - try: - await interaction.followup.send("Session reset~", ephemeral=True) - except Exception as e: - logger.debug("Discord followup failed: %s", e) + await self._run_simple_slash(interaction, "/reset", "Session reset~") @tree.command(name="model", description="Show or change the model") @discord.app_commands.describe(name="Model name (e.g. anthropic/claude-sonnet-4). Leave empty to see current.") async def slash_model(interaction: discord.Interaction, name: str = ""): - await interaction.response.defer(ephemeral=True) - event = self._build_slash_event(interaction, f"/model {name}".strip()) - await self.handle_message(event) - try: - await interaction.followup.send("Done~", ephemeral=True) - except Exception as e: - logger.debug("Discord followup failed: %s", e) + await self._run_simple_slash(interaction, f"/model {name}".strip()) @tree.command(name="reasoning", description="Show or change reasoning effort") @discord.app_commands.describe(effort="Reasoning effort: xhigh, high, medium, low, minimal, or none.") @@ -594,156 +570,66 @@ class DiscordAdapter(BasePlatformAdapter): @tree.command(name="personality", description="Set a personality") @discord.app_commands.describe(name="Personality name. Leave empty to list available.") async def slash_personality(interaction: discord.Interaction, name: str = ""): - await interaction.response.defer(ephemeral=True) - event = self._build_slash_event(interaction, f"/personality {name}".strip()) - await self.handle_message(event) - try: - await interaction.followup.send("Done~", ephemeral=True) - except Exception as e: - logger.debug("Discord followup failed: %s", e) + await self._run_simple_slash(interaction, f"/personality {name}".strip()) @tree.command(name="retry", description="Retry your last message") async def slash_retry(interaction: discord.Interaction): - await interaction.response.defer(ephemeral=True) - event = self._build_slash_event(interaction, "/retry") - await self.handle_message(event) - try: - await interaction.followup.send("Retrying~", ephemeral=True) - except Exception as e: - logger.debug("Discord followup failed: %s", e) + await self._run_simple_slash(interaction, "/retry", "Retrying~") @tree.command(name="undo", description="Remove the last exchange") async def slash_undo(interaction: discord.Interaction): - await interaction.response.defer(ephemeral=True) - event = self._build_slash_event(interaction, "/undo") - await self.handle_message(event) - try: - await interaction.followup.send("Done~", ephemeral=True) - except Exception as e: - logger.debug("Discord followup failed: %s", e) + await self._run_simple_slash(interaction, "/undo") @tree.command(name="status", description="Show Hermes session status") async def slash_status(interaction: discord.Interaction): - await interaction.response.defer(ephemeral=True) - event = self._build_slash_event(interaction, "/status") - await self.handle_message(event) - try: - await interaction.followup.send("Status sent~", ephemeral=True) - except Exception as e: - logger.debug("Discord followup failed: %s", e) + await self._run_simple_slash(interaction, "/status", "Status sent~") @tree.command(name="sethome", description="Set this chat as the home channel") async def slash_sethome(interaction: discord.Interaction): - await interaction.response.defer(ephemeral=True) - event = self._build_slash_event(interaction, "/sethome") - await self.handle_message(event) - try: - await interaction.followup.send("Done~", ephemeral=True) - except Exception as e: - logger.debug("Discord followup failed: %s", e) + await self._run_simple_slash(interaction, "/sethome") @tree.command(name="stop", description="Stop the running Hermes agent") async def slash_stop(interaction: discord.Interaction): - await interaction.response.defer(ephemeral=True) - event = self._build_slash_event(interaction, "/stop") - await self.handle_message(event) - try: - await interaction.followup.send("Stop requested~", ephemeral=True) - except Exception as e: - logger.debug("Discord followup failed: %s", e) + await self._run_simple_slash(interaction, "/stop", "Stop requested~") @tree.command(name="compress", description="Compress conversation context") async def slash_compress(interaction: discord.Interaction): - await interaction.response.defer(ephemeral=True) - event = self._build_slash_event(interaction, "/compress") - await self.handle_message(event) - try: - await interaction.followup.send("Done~", ephemeral=True) - except Exception as e: - logger.debug("Discord followup failed: %s", e) + await self._run_simple_slash(interaction, "/compress") @tree.command(name="title", description="Set or show the session title") @discord.app_commands.describe(name="Session title. Leave empty to show current.") async def slash_title(interaction: discord.Interaction, name: str = ""): - await interaction.response.defer(ephemeral=True) - event = self._build_slash_event(interaction, f"/title {name}".strip()) - await self.handle_message(event) - try: - await interaction.followup.send("Done~", ephemeral=True) - except Exception as e: - logger.debug("Discord followup failed: %s", e) + await self._run_simple_slash(interaction, f"/title {name}".strip()) @tree.command(name="resume", description="Resume a previously-named session") @discord.app_commands.describe(name="Session name to resume. Leave empty to list sessions.") async def slash_resume(interaction: discord.Interaction, name: str = ""): - await interaction.response.defer(ephemeral=True) - event = self._build_slash_event(interaction, f"/resume {name}".strip()) - await self.handle_message(event) - try: - await interaction.followup.send("Done~", ephemeral=True) - except Exception as e: - logger.debug("Discord followup failed: %s", e) + await self._run_simple_slash(interaction, f"/resume {name}".strip()) @tree.command(name="usage", description="Show token usage for this session") async def slash_usage(interaction: discord.Interaction): - await interaction.response.defer(ephemeral=True) - event = self._build_slash_event(interaction, "/usage") - await self.handle_message(event) - try: - await interaction.followup.send("Done~", ephemeral=True) - except Exception as e: - logger.debug("Discord followup failed: %s", e) + await self._run_simple_slash(interaction, "/usage") @tree.command(name="provider", description="Show available providers") async def slash_provider(interaction: discord.Interaction): - await interaction.response.defer(ephemeral=True) - event = self._build_slash_event(interaction, "/provider") - await self.handle_message(event) - try: - await interaction.followup.send("Done~", ephemeral=True) - except Exception as e: - logger.debug("Discord followup failed: %s", e) + await self._run_simple_slash(interaction, "/provider") @tree.command(name="help", description="Show available commands") async def slash_help(interaction: discord.Interaction): - await interaction.response.defer(ephemeral=True) - event = self._build_slash_event(interaction, "/help") - await self.handle_message(event) - try: - await interaction.followup.send("Done~", ephemeral=True) - except Exception as e: - logger.debug("Discord followup failed: %s", e) + await self._run_simple_slash(interaction, "/help") @tree.command(name="insights", description="Show usage insights and analytics") @discord.app_commands.describe(days="Number of days to analyze (default: 7)") async def slash_insights(interaction: discord.Interaction, days: int = 7): - await interaction.response.defer(ephemeral=True) - event = self._build_slash_event(interaction, f"/insights {days}") - await self.handle_message(event) - try: - await interaction.followup.send("Done~", ephemeral=True) - except Exception as e: - logger.debug("Discord followup failed: %s", e) + await self._run_simple_slash(interaction, f"/insights {days}") @tree.command(name="reload-mcp", description="Reload MCP servers from config") async def slash_reload_mcp(interaction: discord.Interaction): - await interaction.response.defer(ephemeral=True) - event = self._build_slash_event(interaction, "/reload-mcp") - await self.handle_message(event) - try: - await interaction.followup.send("Done~", ephemeral=True) - except Exception as e: - logger.debug("Discord followup failed: %s", e) + await self._run_simple_slash(interaction, "/reload-mcp") @tree.command(name="update", description="Update Hermes Agent to the latest version") async def slash_update(interaction: discord.Interaction): - await interaction.response.defer(ephemeral=True) - event = self._build_slash_event(interaction, "/update") - await self.handle_message(event) - try: - await interaction.followup.send("Update initiated~", ephemeral=True) - except Exception as e: - logger.debug("Discord followup failed: %s", e) + await self._run_simple_slash(interaction, "/update", "Update initiated~") @tree.command(name="thread", description="Create a new thread and start a Hermes session in it") @discord.app_commands.describe( diff --git a/gateway/platforms/slack.py b/gateway/platforms/slack.py index aa2da2bf8..d75685bfb 100644 --- a/gateway/platforms/slack.py +++ b/gateway/platforms/slack.py @@ -260,6 +260,30 @@ class SlackAdapter(BasePlatformAdapter): return metadata["thread_ts"] return reply_to + async def _upload_file( + self, + chat_id: str, + file_path: str, + caption: Optional[str] = None, + reply_to: Optional[str] = None, + metadata: Optional[Dict[str, Any]] = None, + ) -> SendResult: + """Upload a local file to Slack.""" + if not self._app: + return SendResult(success=False, error="Not connected") + + if not os.path.exists(file_path): + raise FileNotFoundError(f"File not found: {file_path}") + + result = await self._app.client.files_upload_v2( + channel=chat_id, + file=file_path, + filename=os.path.basename(file_path), + initial_comment=caption or "", + thread_ts=self._resolve_thread_ts(reply_to, metadata), + ) + return SendResult(success=True, raw_response=result) + # ----- Markdown → mrkdwn conversion ----- def format_message(self, content: str) -> str: @@ -417,23 +441,10 @@ class SlackAdapter(BasePlatformAdapter): metadata: Optional[Dict[str, Any]] = None, ) -> SendResult: """Send a local image file to Slack by uploading it.""" - if not self._app: - return SendResult(success=False, error="Not connected") - try: - import os - if not os.path.exists(image_path): - return SendResult(success=False, error=f"Image file not found: {image_path}") - - result = await self._app.client.files_upload_v2( - channel=chat_id, - file=image_path, - filename=os.path.basename(image_path), - initial_comment=caption or "", - thread_ts=self._resolve_thread_ts(reply_to, metadata), - ) - return SendResult(success=True, raw_response=result) - + return await self._upload_file(chat_id, image_path, caption, reply_to, metadata) + except FileNotFoundError: + return SendResult(success=False, error=f"Image file not found: {image_path}") except Exception as e: # pragma: no cover - defensive logging logger.error( "[%s] Failed to send local Slack image %s: %s", @@ -497,19 +508,10 @@ class SlackAdapter(BasePlatformAdapter): metadata: Optional[Dict[str, Any]] = None, ) -> SendResult: """Send an audio file to Slack.""" - if not self._app: - return SendResult(success=False, error="Not connected") - try: - result = await self._app.client.files_upload_v2( - channel=chat_id, - file=audio_path, - filename=os.path.basename(audio_path), - initial_comment=caption or "", - thread_ts=self._resolve_thread_ts(reply_to, metadata), - ) - return SendResult(success=True, raw_response=result) - + return await self._upload_file(chat_id, audio_path, caption, reply_to, metadata) + except FileNotFoundError: + return SendResult(success=False, error=f"Audio file not found: {audio_path}") except Exception as e: # pragma: no cover - defensive logging logger.error( "[Slack] Failed to send audio file %s: %s", diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 4a31db808..98c204e68 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -69,6 +69,8 @@ os.environ.setdefault("MSWEA_GLOBAL_CONFIG_DIR", str(get_hermes_home())) os.environ.setdefault("MSWEA_SILENT_STARTUP", "1") import logging +import time as _time +from datetime import datetime from hermes_cli import __version__, __release_date__ from hermes_constants import OPENROUTER_BASE_URL @@ -76,6 +78,24 @@ from hermes_constants import OPENROUTER_BASE_URL logger = logging.getLogger(__name__) +def _relative_time(ts) -> str: + """Format a timestamp as relative time (e.g., '2h ago', 'yesterday').""" + if not ts: + return "?" + delta = _time.time() - ts + if delta < 60: + return "just now" + if delta < 3600: + return f"{int(delta / 60)}m ago" + if delta < 86400: + return f"{int(delta / 3600)}h ago" + if delta < 172800: + return "yesterday" + if delta < 604800: + return f"{int(delta / 86400)}d ago" + return datetime.fromtimestamp(ts).strftime("%Y-%m-%d") + + def _has_any_provider_configured() -> bool: """Check if at least one inference provider is usable.""" from hermes_cli.config import get_env_path, get_hermes_home @@ -140,28 +160,9 @@ def _session_browse_picker(sessions: list) -> Optional[str]: # Try curses-based picker first try: import curses - import time as _time - from datetime import datetime result_holder = [None] - def _relative_time(ts): - if not ts: - return "?" - delta = _time.time() - ts - if delta < 60: - return "just now" - elif delta < 3600: - return f"{int(delta / 60)}m ago" - elif delta < 86400: - return f"{int(delta / 3600)}h ago" - elif delta < 172800: - return "yesterday" - elif delta < 604800: - return f"{int(delta / 86400)}d ago" - else: - return datetime.fromtimestamp(ts).strftime("%Y-%m-%d") - def _format_row(s, max_x): """Format a session row for display.""" title = (s.get("title") or "").strip() @@ -352,26 +353,6 @@ def _session_browse_picker(sessions: list) -> Optional[str]: pass # Fallback: numbered list (Windows without curses, etc.) - import time as _time - from datetime import datetime - - def _relative_time_fb(ts): - if not ts: - return "?" - delta = _time.time() - ts - if delta < 60: - return "just now" - elif delta < 3600: - return f"{int(delta / 60)}m ago" - elif delta < 86400: - return f"{int(delta / 3600)}h ago" - elif delta < 172800: - return "yesterday" - elif delta < 604800: - return f"{int(delta / 86400)}d ago" - else: - return datetime.fromtimestamp(ts).strftime("%Y-%m-%d") - print("\n Browse sessions (enter number to resume, q to cancel)\n") for i, s in enumerate(sessions): title = (s.get("title") or "").strip() @@ -379,7 +360,7 @@ def _session_browse_picker(sessions: list) -> Optional[str]: label = title or preview or s["id"] if len(label) > 50: label = label[:47] + "..." - last_active = _relative_time_fb(s.get("last_active")) + last_active = _relative_time(s.get("last_active")) src = s.get("source", "")[:6] print(f" {i + 1:>3}. {label:<50} {last_active:<10} {src}") @@ -2846,30 +2827,6 @@ For more help on a command: if not sessions: print("No sessions found.") return - from datetime import datetime - import time as _time - - def _relative_time(ts): - """Format a timestamp as relative time (e.g., '2h ago', 'yesterday').""" - if not ts: - return "?" - delta = _time.time() - ts - if delta < 60: - return "just now" - elif delta < 3600: - mins = int(delta / 60) - return f"{mins}m ago" - elif delta < 86400: - hours = int(delta / 3600) - return f"{hours}h ago" - elif delta < 172800: - return "yesterday" - elif delta < 604800: - days = int(delta / 86400) - return f"{days}d ago" - else: - return datetime.fromtimestamp(ts).strftime("%Y-%m-%d") - has_titles = any(s.get("title") for s in sessions) if has_titles: print(f"{'Title':<22} {'Preview':<40} {'Last Active':<13} {'ID'}") diff --git a/hermes_state.py b/hermes_state.py index 84c3bf44a..5e29321ec 100644 --- a/hermes_state.py +++ b/hermes_state.py @@ -267,8 +267,6 @@ class SessionDB: if not title: return None - import re - # Remove ASCII control characters (0x00-0x1F, 0x7F) but keep # whitespace chars (\t=0x09, \n=0x0A, \r=0x0D) so they can be # normalized to spaces by the whitespace collapsing step below @@ -373,7 +371,6 @@ class SessionDB: Strips any existing " #N" suffix to find the base name, then finds the highest existing number and increments. """ - import re # Strip existing #N suffix to find the true base match = re.match(r'^(.*?) #(\d+)$', base_title) if match: diff --git a/tests/gateway/test_send_image_file.py b/tests/gateway/test_send_image_file.py index 9fa375ac2..bf2437345 100644 --- a/tests/gateway/test_send_image_file.py +++ b/tests/gateway/test_send_image_file.py @@ -297,7 +297,9 @@ class TestScreenshotCleanup: def test_cleanup_removes_old_screenshots(self, tmp_path): """_cleanup_old_screenshots should remove files older than max_age_hours.""" import time - from tools.browser_tool import _cleanup_old_screenshots + from tools.browser_tool import _cleanup_old_screenshots, _last_screenshot_cleanup_by_dir + + _last_screenshot_cleanup_by_dir.clear() # Create a "fresh" file fresh = tmp_path / "browser_screenshot_fresh.png" @@ -314,10 +316,32 @@ class TestScreenshotCleanup: assert fresh.exists(), "Fresh screenshot should not be removed" assert not old.exists(), "Old screenshot should be removed" + def test_cleanup_is_throttled_per_directory(self, tmp_path): + import time + from tools.browser_tool import _cleanup_old_screenshots, _last_screenshot_cleanup_by_dir + + _last_screenshot_cleanup_by_dir.clear() + + old = tmp_path / "browser_screenshot_old.png" + old.write_bytes(b"old") + old_time = time.time() - (25 * 3600) + os.utime(str(old), (old_time, old_time)) + + _cleanup_old_screenshots(tmp_path, max_age_hours=24) + assert not old.exists() + + old.write_bytes(b"old-again") + os.utime(str(old), (old_time, old_time)) + _cleanup_old_screenshots(tmp_path, max_age_hours=24) + + assert old.exists(), "Repeated cleanup should be skipped while throttled" + def test_cleanup_ignores_non_screenshot_files(self, tmp_path): """Only files matching browser_screenshot_*.png should be cleaned.""" import time - from tools.browser_tool import _cleanup_old_screenshots + from tools.browser_tool import _cleanup_old_screenshots, _last_screenshot_cleanup_by_dir + + _last_screenshot_cleanup_by_dir.clear() other_file = tmp_path / "important_data.txt" other_file.write_bytes(b"keep me") @@ -330,11 +354,13 @@ class TestScreenshotCleanup: def test_cleanup_handles_empty_dir(self, tmp_path): """Cleanup should not fail on empty directory.""" - from tools.browser_tool import _cleanup_old_screenshots + from tools.browser_tool import _cleanup_old_screenshots, _last_screenshot_cleanup_by_dir + _last_screenshot_cleanup_by_dir.clear() _cleanup_old_screenshots(tmp_path, max_age_hours=24) # Should not raise def test_cleanup_handles_nonexistent_dir(self): """Cleanup should not fail if directory doesn't exist.""" from pathlib import Path - from tools.browser_tool import _cleanup_old_screenshots + from tools.browser_tool import _cleanup_old_screenshots, _last_screenshot_cleanup_by_dir + _last_screenshot_cleanup_by_dir.clear() _cleanup_old_screenshots(Path("/nonexistent/dir"), max_age_hours=24) # Should not raise diff --git a/tools/browser_tool.py b/tools/browser_tool.py index ae9515748..15f496189 100644 --- a/tools/browser_tool.py +++ b/tools/browser_tool.py @@ -67,6 +67,12 @@ from agent.auxiliary_client import call_llm logger = logging.getLogger(__name__) +# Standard PATH entries for environments with minimal PATH (e.g. systemd services) +_SANE_PATH = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" + +# Throttle screenshot cleanup to avoid repeated full directory scans. +_last_screenshot_cleanup_by_dir: dict[str, float] = {} + # ============================================================================ # Configuration # ============================================================================ @@ -846,7 +852,6 @@ def _run_browser_command( browser_env = {**os.environ} # Ensure PATH includes standard dirs (systemd services may have minimal PATH) - _SANE_PATH = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" if "/usr/bin" not in browser_env.get("PATH", "").split(":"): browser_env["PATH"] = f"{browser_env.get('PATH', '')}:{_SANE_PATH}" browser_env["AGENT_BROWSER_SOCKET_DIR"] = task_socket_dir @@ -1578,8 +1583,17 @@ def browser_vision(question: str, annotate: bool = False, task_id: Optional[str] def _cleanup_old_screenshots(screenshots_dir, max_age_hours=24): - """Remove browser screenshots older than max_age_hours to prevent disk bloat.""" - import time + """Remove browser screenshots older than max_age_hours to prevent disk bloat. + + Throttled to run at most once per hour per directory to avoid repeated + scans on screenshot-heavy workflows. + """ + key = str(screenshots_dir) + now = time.time() + if now - _last_screenshot_cleanup_by_dir.get(key, 0.0) < 3600: + return + _last_screenshot_cleanup_by_dir[key] = now + try: cutoff = time.time() - (max_age_hours * 3600) for f in screenshots_dir.glob("browser_screenshot_*.png"):