From e480ce9fb88bec5465e6d64890738a9d08934a07 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 22 Sep 2026 15:31:28 +0000 Subject: [PATCH] refactor: sever the agentic-mode imports from the classic handlers Agentic mode pulled _format_error_message and _update_working_directory_from_claude_response out of src/bot/handlers/message.py at seven call sites, and registered restart_command and sync_threads from src/bot/handlers/command.py. That is what made classic mode undeletable. The two helpers move to src/bot/utils/error_messages.py and src/bot/utils/working_directory.py. The two commands, which both modes register, move to src/bot/commands.py together with the _is_private_chat predicate sync_threads needs. Classic mode imports all of them from the new homes, so behaviour is unchanged in either mode. The only `from .handlers` import left in orchestrator.py is the classic registration inside _register_classic_handlers. A new test parses the orchestrator's AST and fails if another one appears, and checks that the three new shared modules do not reach back into handlers/. Tests that patched the helpers on handlers.message now patch the names the orchestrator binds at module level. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Uta5V6wHoXYU3muwEpvaDt --- CHANGELOG.md | 3 + CLAUDE.md | 4 + src/bot/commands.py | 155 ++++++++++ src/bot/handlers/command.py | 139 +-------- src/bot/handlers/message.py | 267 +----------------- src/bot/orchestrator.py | 26 +- src/bot/utils/error_messages.py | 213 ++++++++++++++ src/bot/utils/working_directory.py | 70 +++++ tests/unit/test_bot/test_agentic_imports.py | 60 ++++ tests/unit/test_bot/test_stop_button.py | 6 +- .../test_bot/test_thread_mode_handlers.py | 10 +- tests/unit/test_orchestrator.py | 4 +- 12 files changed, 527 insertions(+), 430 deletions(-) create mode 100644 src/bot/commands.py create mode 100644 src/bot/utils/error_messages.py create mode 100644 src/bot/utils/working_directory.py create mode 100644 tests/unit/test_bot/test_agentic_imports.py diff --git a/CHANGELOG.md b/CHANGELOG.md index af2807cee..8e881526b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed +- **Agentic mode no longer imports from the classic handlers**: `src/bot/orchestrator.py` pulled `_format_error_message` and `_update_working_directory_from_claude_response` out of `src/bot/handlers/message.py` at seven call sites, and registered `restart_command` and `sync_threads` from `src/bot/handlers/command.py`, which is what made classic mode undeletable. The two helpers now live in `src/bot/utils/error_messages.py` and `src/bot/utils/working_directory.py`; the two commands, which both modes register, live in `src/bot/commands.py`. Classic mode imports all four from the new homes, so nothing changes for either mode. The only `from .handlers` import left in the orchestrator is the classic registration inside `_register_classic_handlers`, and a new test walks the orchestrator's AST so another one cannot slip back in. This is the groundwork for classic-mode removal, roadmap item 4.1 in `docs/ROADMAP-v2.md` + ## [1.8.0] - 2026-09-22 Released as a minor rather than a patch: `claude-agent-sdk` moves from the 0.1 diff --git a/CLAUDE.md b/CLAUDE.md index c7b24d168..ef1951047 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -71,6 +71,8 @@ context.bot_data["security_validator"] - `src/bot/middleware/` -- Auth, rate limit, security input validation - `src/bot/features/` -- Git integration, file handling, quick actions, session export - `src/bot/orchestrator.py` -- MessageOrchestrator: routes to agentic or classic handlers, project-topic routing +- `src/bot/commands.py` -- commands both modes register (`/restart`, `/sync_threads`); not classic-mode code +- `src/bot/utils/` -- formatting, HTML escaping, image extraction, error rendering (`error_messages.py`), working-directory tracking (`working_directory.py`), shared by both modes - `src/claude/` -- Claude integration facade, SDK/CLI managers, session management, tool monitoring - `src/projects/` -- Multi-project support: `registry.py` (YAML project config), `thread_manager.py` (Telegram topic sync/routing) - `src/storage/` -- SQLite via aiosqlite, repository pattern (users, sessions, messages, tool_usage, audit_log, cost_tracking, project_threads) @@ -139,6 +141,8 @@ Agentic mode commands: `/start`, `/new`, `/status`, `/verbose`, `/repo`. If `ENA ### Classic mode +Classic mode is scheduled for removal in 2.0 (roadmap item 4.1). Agentic code must not import from `src/bot/handlers/`: anything both modes need lives in `src/bot/commands.py` or `src/bot/utils/`, and `tests/unit/test_bot/test_agentic_imports.py` fails if `orchestrator.py` gains another `from .handlers` import. + 1. Add handler function in `src/bot/handlers/command.py` 2. Register in `MessageOrchestrator._register_classic_handlers()` 3. Add to `MessageOrchestrator.get_bot_commands()` for Telegram's command menu diff --git a/src/bot/commands.py b/src/bot/commands.py new file mode 100644 index 000000000..afdc241dc --- /dev/null +++ b/src/bot/commands.py @@ -0,0 +1,155 @@ +"""Commands registered in both agentic and classic mode. + +``/restart`` and ``/sync_threads`` are not classic-mode features: the agentic +orchestrator registers them too. They live here, outside +``src/bot/handlers/``, so that agentic mode does not import from the classic +handlers package. +""" + +import os +import signal + +import structlog +from telegram import Update +from telegram.ext import ContextTypes + +from ..config.settings import Settings +from ..projects import PrivateTopicsUnavailableError, load_project_registry +from ..security.audit import AuditLogger +from .utils.html_format import escape_html + +logger = structlog.get_logger() + + +def _is_private_chat(update: Update) -> bool: + """Return True when update is from a private chat.""" + chat = update.effective_chat + return bool(chat and getattr(chat, "type", "") == "private") + + +async def sync_threads(update: Update, context: ContextTypes.DEFAULT_TYPE) -> None: + """Synchronize project topics in the configured forum chat.""" + settings: Settings = context.bot_data["settings"] + audit_logger: AuditLogger = context.bot_data.get("audit_logger") + user_id = update.effective_user.id + + if not settings.enable_project_threads: + await update.message.reply_text( + "ℹ️ Project thread mode is disabled.", parse_mode="HTML" + ) + return + + manager = context.bot_data.get("project_threads_manager") + if not manager: + await update.message.reply_text( + "❌ Project thread manager not initialized.", parse_mode="HTML" + ) + return + + status_msg = await update.message.reply_text( + "🔄 Syncing project topics...", parse_mode="HTML" + ) + + if settings.project_threads_mode == "private": + if not _is_private_chat(update): + await status_msg.edit_text( + "❌ Private Thread Mode\n\n" + "Run /sync_threads in your private chat with the bot.", + parse_mode="HTML", + ) + return + target_chat_id = update.effective_chat.id + else: + if settings.project_threads_chat_id is None: + await status_msg.edit_text( + "❌ Group Thread Mode Misconfigured\n\n" + "Set PROJECT_THREADS_CHAT_ID first.", + parse_mode="HTML", + ) + return + if ( + not update.effective_chat + or update.effective_chat.id != settings.project_threads_chat_id + ): + await status_msg.edit_text( + "❌ Group Thread Mode\n\n" + "Run /sync_threads in the configured project threads group.", + parse_mode="HTML", + ) + return + target_chat_id = settings.project_threads_chat_id + + try: + if not settings.projects_config_path: + await status_msg.edit_text( + "❌ Project thread mode is misconfigured\n\n" + "Set PROJECTS_CONFIG_PATH to a valid YAML file.", + parse_mode="HTML", + ) + if audit_logger: + await audit_logger.log_command(user_id, "sync_threads", [], False) + return + + registry = load_project_registry( + config_path=settings.projects_config_path, + approved_directory=settings.approved_directory, + ) + manager.registry = registry + context.bot_data["project_registry"] = registry + + result = await manager.sync_topics(context.bot, chat_id=target_chat_id) + await status_msg.edit_text( + "✅ Project topic sync complete\n\n" + f"• Created: {result.created}\n" + f"• Reused: {result.reused}\n" + f"• Renamed: {result.renamed}\n" + f"• Reopened: {result.reopened}\n" + f"• Closed: {result.closed}\n" + f"• Deactivated: {result.deactivated}\n" + f"• Failed: {result.failed}", + parse_mode="HTML", + ) + if audit_logger: + await audit_logger.log_command(user_id, "sync_threads", [], True) + except PrivateTopicsUnavailableError: + await status_msg.edit_text( + manager.private_topics_unavailable_message(), + parse_mode="HTML", + ) + if audit_logger: + await audit_logger.log_command(user_id, "sync_threads", [], False) + except Exception as e: + await status_msg.edit_text( + f"❌ Project topic sync failed\n\n{escape_html(str(e))}", + parse_mode="HTML", + ) + if audit_logger: + await audit_logger.log_command(user_id, "sync_threads", [], False) + + +async def restart_command(update: Update, context: ContextTypes.DEFAULT_TYPE) -> None: + """Handle /restart command - gracefully restart the bot process. + + Sends a confirmation message then triggers SIGTERM so systemd + (or any process manager with restart-on-exit) brings the bot back up. + + Auth: protected by the auth middleware (group -2) which raises + ``ApplicationHandlerStop`` for unauthenticated users before any + handler in group 10 runs. No per-handler check is needed. + """ + audit_logger: AuditLogger = context.bot_data.get("audit_logger") + user_id = update.effective_user.id + + await update.message.reply_text( + "🔄 Restarting bot…\n\nBack shortly.", + parse_mode="HTML", + ) + + if audit_logger: + await audit_logger.log_command(user_id, "restart", [], True) + + logger.info("Restart requested via /restart command", user_id=user_id) + + # SIGTERM triggers the existing graceful-shutdown handler in main.py; + # systemd Restart=always will bring the process back up. + os.kill(os.getpid(), signal.SIGTERM) diff --git a/src/bot/handlers/command.py b/src/bot/handlers/command.py index 651a08f8c..5306efa20 100644 --- a/src/bot/handlers/command.py +++ b/src/bot/handlers/command.py @@ -1,7 +1,5 @@ """Command handlers for bot operations.""" -import os -import signal from datetime import datetime, timezone from pathlib import Path from typing import Optional @@ -12,10 +10,11 @@ from ...claude.facade import ClaudeIntegration from ...config.settings import Settings -from ...projects import PrivateTopicsUnavailableError, load_project_registry +from ...projects import PrivateTopicsUnavailableError from ...security.audit import AuditLogger from ...security.validators import SecurityValidator from ...storage.models import SessionModel +from ..commands import _is_private_chat from ..utils.html_format import escape_html logger = structlog.get_logger() @@ -42,12 +41,6 @@ def _get_thread_project_root( return Path(thread_context["project_root"]).resolve() -def _is_private_chat(update: Update) -> bool: - """Return True when update is from a private chat.""" - chat = update.effective_chat - return bool(chat and getattr(chat, "type", "") == "private") - - async def start_command(update: Update, context: ContextTypes.DEFAULT_TYPE) -> None: """Handle /start command.""" user = update.effective_user @@ -205,106 +198,6 @@ async def help_command(update: Update, context: ContextTypes.DEFAULT_TYPE) -> No await update.message.reply_text(help_text, parse_mode="HTML") -async def sync_threads(update: Update, context: ContextTypes.DEFAULT_TYPE) -> None: - """Synchronize project topics in the configured forum chat.""" - settings: Settings = context.bot_data["settings"] - audit_logger: AuditLogger = context.bot_data.get("audit_logger") - user_id = update.effective_user.id - - if not settings.enable_project_threads: - await update.message.reply_text( - "ℹ️ Project thread mode is disabled.", parse_mode="HTML" - ) - return - - manager = context.bot_data.get("project_threads_manager") - if not manager: - await update.message.reply_text( - "❌ Project thread manager not initialized.", parse_mode="HTML" - ) - return - - status_msg = await update.message.reply_text( - "🔄 Syncing project topics...", parse_mode="HTML" - ) - - if settings.project_threads_mode == "private": - if not _is_private_chat(update): - await status_msg.edit_text( - "❌ Private Thread Mode\n\n" - "Run /sync_threads in your private chat with the bot.", - parse_mode="HTML", - ) - return - target_chat_id = update.effective_chat.id - else: - if settings.project_threads_chat_id is None: - await status_msg.edit_text( - "❌ Group Thread Mode Misconfigured\n\n" - "Set PROJECT_THREADS_CHAT_ID first.", - parse_mode="HTML", - ) - return - if ( - not update.effective_chat - or update.effective_chat.id != settings.project_threads_chat_id - ): - await status_msg.edit_text( - "❌ Group Thread Mode\n\n" - "Run /sync_threads in the configured project threads group.", - parse_mode="HTML", - ) - return - target_chat_id = settings.project_threads_chat_id - - try: - if not settings.projects_config_path: - await status_msg.edit_text( - "❌ Project thread mode is misconfigured\n\n" - "Set PROJECTS_CONFIG_PATH to a valid YAML file.", - parse_mode="HTML", - ) - if audit_logger: - await audit_logger.log_command(user_id, "sync_threads", [], False) - return - - registry = load_project_registry( - config_path=settings.projects_config_path, - approved_directory=settings.approved_directory, - ) - manager.registry = registry - context.bot_data["project_registry"] = registry - - result = await manager.sync_topics(context.bot, chat_id=target_chat_id) - await status_msg.edit_text( - "✅ Project topic sync complete\n\n" - f"• Created: {result.created}\n" - f"• Reused: {result.reused}\n" - f"• Renamed: {result.renamed}\n" - f"• Reopened: {result.reopened}\n" - f"• Closed: {result.closed}\n" - f"• Deactivated: {result.deactivated}\n" - f"• Failed: {result.failed}", - parse_mode="HTML", - ) - if audit_logger: - await audit_logger.log_command(user_id, "sync_threads", [], True) - except PrivateTopicsUnavailableError: - await status_msg.edit_text( - manager.private_topics_unavailable_message(), - parse_mode="HTML", - ) - if audit_logger: - await audit_logger.log_command(user_id, "sync_threads", [], False) - except Exception as e: - await status_msg.edit_text( - f"❌ Project topic sync failed\n\n{escape_html(str(e))}", - parse_mode="HTML", - ) - if audit_logger: - await audit_logger.log_command(user_id, "sync_threads", [], False) - - async def new_session(update: Update, context: ContextTypes.DEFAULT_TYPE) -> None: """Handle /new command - explicitly starts a fresh session, clearing previous context.""" settings: Settings = context.bot_data["settings"] @@ -1232,34 +1125,6 @@ async def git_command(update: Update, context: ContextTypes.DEFAULT_TYPE) -> Non logger.error("Error in git_command", error=str(e), user_id=user_id) -async def restart_command(update: Update, context: ContextTypes.DEFAULT_TYPE) -> None: - """Handle /restart command - gracefully restart the bot process. - - Sends a confirmation message then triggers SIGTERM so systemd - (or any process manager with restart-on-exit) brings the bot back up. - - Auth: protected by the auth middleware (group -2) which raises - ``ApplicationHandlerStop`` for unauthenticated users before any - handler in group 10 runs. No per-handler check is needed. - """ - audit_logger: AuditLogger = context.bot_data.get("audit_logger") - user_id = update.effective_user.id - - await update.message.reply_text( - "🔄 Restarting bot…\n\nBack shortly.", - parse_mode="HTML", - ) - - if audit_logger: - await audit_logger.log_command(user_id, "restart", [], True) - - logger.info("Restart requested via /restart command", user_id=user_id) - - # SIGTERM triggers the existing graceful-shutdown handler in main.py; - # systemd Restart=always will bring the process back up. - os.kill(os.getpid(), signal.SIGTERM) - - def _format_file_size(size: int) -> str: """Format file size in human-readable format.""" for unit in ["B", "KB", "MB", "GB"]: diff --git a/src/bot/handlers/message.py b/src/bot/handlers/message.py index bbd240840..b1dbeff13 100644 --- a/src/bot/handlers/message.py +++ b/src/bot/handlers/message.py @@ -7,24 +7,18 @@ from telegram import InputMediaPhoto, Update from telegram.ext import ContextTypes -from ...claude.exceptions import ( - ClaudeError, - ClaudeMCPError, - ClaudeParsingError, - ClaudeProcessError, - ClaudeSessionError, - ClaudeTimeoutError, -) from ...config.settings import Settings from ...security.audit import AuditLogger from ...security.rate_limiter import RateLimiter from ...security.validators import SecurityValidator +from ..utils.error_messages import _format_error_message from ..utils.html_format import escape_html from ..utils.image_extractor import ( ImageAttachment, should_send_as_photo, validate_image_path, ) +from ..utils.working_directory import _update_working_directory_from_claude_response logger = structlog.get_logger() @@ -96,203 +90,6 @@ async def _format_progress_update(update_obj) -> Optional[str]: return None -def _format_error_message(error: Exception | str) -> str: - """Format error messages for user-friendly display. - - Accepts an exception object (preferred) or a string for backward - compatibility. When an exception is provided, the error type is used - to produce a specific, actionable message. - """ - # Normalise: keep both the object and a string representation. - if isinstance(error, str): - error_str = error - error_obj: Exception | None = None - else: - error_str = str(error) - error_obj = error - - # --- Dispatch on exception type first (most specific) --- - - if isinstance(error_obj, ClaudeTimeoutError): - return ( - "⏰ Request Timeout\n\n" - f"{escape_html(error_str)}\n\n" - "What you can do:\n" - "• Try breaking your request into smaller parts\n" - "• Avoid asking for very large file operations in one go\n" - "• Try again — transient slowdowns happen" - ) - - if isinstance(error_obj, ClaudeMCPError): - server_hint = "" - if error_obj.server_name: - server_hint = f" ({escape_html(error_obj.server_name)})" - return ( - f"🔌 MCP Server Error{server_hint}\n\n" - f"{escape_html(error_str)}\n\n" - "What you can do:\n" - "• Check that the MCP server is running and reachable\n" - "• Verify MCP_CONFIG_PATH points to a valid config\n" - "• Ask the administrator to check MCP server logs" - ) - - if isinstance(error_obj, ClaudeParsingError): - return ( - "📄 Response Parsing Error\n\n" - f"Claude returned a response that could not be parsed:\n" - f"{escape_html(error_str[:300])}\n\n" - "What you can do:\n" - "• Try your request again\n" - "• Rephrase your prompt if the problem persists" - ) - - if isinstance(error_obj, ClaudeSessionError): - return ( - "🔄 Session Error\n\n" - f"{escape_html(error_str)}\n\n" - "What you can do:\n" - "• Use /new to start a fresh session\n" - "• Try your request again\n" - "• Use /status to check your current session" - ) - - if isinstance(error_obj, ClaudeProcessError): - return _format_process_error(error_str) - - # Any future ClaudeError subtypes not explicitly handled above — - # preserve their existing message as-is rather than downgrading - # to a generic "process error". - if isinstance(error_obj, ClaudeError): - safe_error = escape_html(error_str) - if len(safe_error) > 500: - safe_error = safe_error[:500] + "..." - return ( - f"❌ Claude Error\n\n" - f"{safe_error}\n\n" - f"Try again or use /new to start a fresh session." - ) - - # --- Fall back to keyword matching (for string-only callers) -------- - # These patterns match the known error prefixes produced by - # sdk_integration.py and facade.py, NOT arbitrary user content. - - error_lower = error_str.lower() - - if "usage limit reached" in error_lower or "usage limit" in error_lower: - return error_str # Already user-friendly - - if "tool not allowed" in error_lower: - return error_str # Already formatted by facade.py - - if "no conversation found" in error_lower: - return ( - "🔄 Session Not Found\n\n" - "The previous Claude session could not be found or has expired.\n\n" - "What you can do:\n" - "• Use /new to start a fresh session\n" - "• Try your request again\n" - "• Use /status to check your current session" - ) - - if "rate limit" in error_lower: - return ( - "⏱️ Rate Limit Reached\n\n" - "Too many requests in a short time period.\n\n" - "What you can do:\n" - "• Wait a moment before trying again\n" - "• Use simpler requests\n" - "• Check your current usage with /status" - ) - - if "timed out after" in error_lower or "claude sdk timed out" in error_lower: - return ( - "⏰ Request Timeout\n\n" - f"{escape_html(error_str)}\n\n" - "What you can do:\n" - "• Try breaking your request into smaller parts\n" - "• Avoid asking for very large file operations in one go\n" - "• Try again — transient slowdowns happen" - ) - - if "overloaded" in error_lower: - return ( - "🏗️ Claude is Overloaded\n\n" - "The Claude API is currently experiencing high demand.\n\n" - "What you can do:\n" - "• Wait a moment and try again\n" - "• Shorter prompts may succeed more easily" - ) - - if "invalid api key" in error_lower or "authentication_error" in error_lower: - return ( - "🔑 API Authentication Error\n\n" - "The API key used to connect to Claude is invalid or expired.\n\n" - "What you can do:\n" - "• Ask the administrator to verify the " - "ANTHROPIC_API_KEY setting\n" - "• Check that the API key has not been revoked" - ) - - # Match known SDK prefixes: "Failed to connect to Claude: ..." - # and "MCP server connection failed: ..." - if error_lower.startswith("failed to connect to claude"): - return ( - "🌐 Connection Error\n\n" - f"Could not connect to Claude:\n" - f"{escape_html(error_str[:300])}\n\n" - "What you can do:\n" - "• Check your network / firewall settings\n" - "• Verify the Claude CLI is installed and accessible\n" - "• Try again in a moment" - ) - - # Match known SDK prefix: "Claude Code not found. ..." - if error_lower.startswith("claude code not found"): - return ( - "🔍 Claude CLI Not Found\n\n" - f"{escape_html(error_str)}\n\n" - "What you can do:\n" - "• Ensure Claude Code is installed: " - "npm install -g @anthropic-ai/claude-code\n" - "• Set the CLAUDE_CLI_PATH environment variable" - ) - - # Match known SDK prefixes: "MCP server error: ..." and - # "MCP server connection failed: ..." - if error_lower.startswith("mcp server"): - return ( - "🔌 MCP Server Error\n\n" - f"{escape_html(error_str)}\n\n" - "What you can do:\n" - "• Check that the MCP server is running\n" - "• Verify MCP configuration\n" - "• Ask the administrator to check MCP server logs" - ) - - # --- No match — show the raw error as-is --- - safe_error = escape_html(error_str) - if len(safe_error) > 500: - safe_error = safe_error[:500] + "..." - - return f"❌ {safe_error}" - - -def _format_process_error(error_str: str) -> str: - """Format a Claude process/SDK error with the actual details.""" - safe_error = escape_html(error_str) - if len(safe_error) > 500: - safe_error = safe_error[:500] + "..." - - return ( - f"❌ Claude Process Error\n\n" - f"{safe_error}\n\n" - "What you can do:\n" - "• Try your request again\n" - "• Use /new to start a fresh session if the problem persists\n" - "• Check /status for current session state" - ) - - async def handle_text_message( update: Update, context: ContextTypes.DEFAULT_TYPE ) -> None: @@ -1226,63 +1023,3 @@ async def _generate_placeholder_response( ) return {"text": response_text, "parse_mode": "HTML"} - - -def _update_working_directory_from_claude_response( - claude_response, context, settings, user_id -): - """Update the working directory based on Claude's response content.""" - import re - from pathlib import Path - - # Look for directory changes in Claude's response - # This searches for common patterns that indicate directory changes - patterns = [ - r"(?:^|\n).*?cd\s+([^\s\n]+)", # cd command - r"(?:^|\n).*?Changed directory to:?\s*([^\s\n]+)", # explicit directory change - r"(?:^|\n).*?Current directory:?\s*([^\s\n]+)", # current directory indication - r"(?:^|\n).*?Working directory:?\s*([^\s\n]+)", # working directory indication - ] - - content = claude_response.content.lower() - current_dir = context.user_data.get( - "current_directory", settings.approved_directory - ) - - for pattern in patterns: - matches = re.findall(pattern, content, re.MULTILINE | re.IGNORECASE) - for match in matches: - try: - # Clean up the path - new_path = match.strip().strip("\"'`") - - # Handle relative paths - if new_path.startswith("./") or new_path.startswith("../"): - new_path = (current_dir / new_path).resolve() - elif not new_path.startswith("/"): - # Relative path without ./ - new_path = (current_dir / new_path).resolve() - else: - # Absolute path - new_path = Path(new_path).resolve() - - # Validate that the new path is within the approved directory - if ( - new_path.is_relative_to(settings.approved_directory) - and new_path.exists() - ): - context.user_data["current_directory"] = new_path - logger.info( - "Updated working directory from Claude response", - old_dir=str(current_dir), - new_dir=str(new_path), - user_id=user_id, - ) - return # Take the first valid match - - except (ValueError, OSError) as e: - # Invalid path, skip this match - logger.debug( - "Invalid path in Claude response", path=match, error=str(e) - ) - continue diff --git a/src/bot/orchestrator.py b/src/bot/orchestrator.py index 5436ba199..2506cd65d 100644 --- a/src/bot/orchestrator.py +++ b/src/bot/orchestrator.py @@ -33,13 +33,16 @@ from ..claude.sdk_integration import StreamUpdate from ..config.settings import Settings from ..projects import PrivateTopicsUnavailableError +from .commands import restart_command, sync_threads from .utils.draft_streamer import DraftStreamer, generate_draft_id +from .utils.error_messages import _format_error_message from .utils.html_format import escape_html from .utils.image_extractor import ( ImageAttachment, should_send_as_photo, validate_image_path, ) +from .utils.working_directory import _update_working_directory_from_claude_response logger = structlog.get_logger() @@ -328,8 +331,6 @@ def register_handlers(self, app: Application) -> None: def _register_agentic_handlers(self, app: Application) -> None: """Register agentic handlers: commands + text/file/photo.""" - from .handlers import command - # Commands handlers = [ ("start", self.agentic_start), @@ -337,10 +338,10 @@ def _register_agentic_handlers(self, app: Application) -> None: ("status", self.agentic_status), ("verbose", self.agentic_verbose), ("repo", self.agentic_repo), - ("restart", command.restart_command), + ("restart", restart_command), ] if self.settings.enable_project_threads: - handlers.append(("sync_threads", command.sync_threads)) + handlers.append(("sync_threads", sync_threads)) # Derive known commands dynamically — avoids drift when new commands are added self._known_commands: frozenset[str] = frozenset(cmd for cmd, _ in handlers) @@ -434,10 +435,10 @@ def _register_classic_handlers(self, app: Application) -> None: ("export", command.export_session), ("actions", command.quick_actions), ("git", command.git_command), - ("restart", command.restart_command), + ("restart", restart_command), ] if self.settings.enable_project_threads: - handlers.append(("sync_threads", command.sync_threads)) + handlers.append(("sync_threads", sync_threads)) for cmd, handler in handlers: app.add_handler(CommandHandler(cmd, self._inject_deps(handler))) @@ -1092,8 +1093,6 @@ async def agentic_text( context.user_data["claude_session_id"] = claude_response.session_id # Track directory changes - from .handlers.message import _update_working_directory_from_claude_response - _update_working_directory_from_claude_response( claude_response, context, self.settings, user_id ) @@ -1128,7 +1127,6 @@ async def agentic_text( except Exception as e: success = False logger.error("Claude integration failed", error=str(e), user_id=user_id) - from .handlers.message import _format_error_message from .utils.formatting import FormattedMessage formatted_messages = [ @@ -1340,8 +1338,6 @@ async def agentic_document( context.user_data["claude_session_id"] = claude_response.session_id - from .handlers.message import _update_working_directory_from_claude_response - _update_working_directory_from_claude_response( claude_response, context, self.settings, user_id ) @@ -1400,8 +1396,6 @@ async def agentic_document( logger.warning("Image send failed", error=str(img_err)) except Exception as e: - from .handlers.message import _format_error_message - await progress_msg.edit_text(_format_error_message(e), parse_mode="HTML") logger.error("Claude file processing failed", error=str(e), user_id=user_id) finally: @@ -1448,8 +1442,6 @@ async def agentic_photo( ) except Exception as e: - from .handlers.message import _format_error_message - await progress_msg.edit_text(_format_error_message(e), parse_mode="HTML") logger.error( "Claude photo processing failed", error=str(e), user_id=user_id @@ -1489,8 +1481,6 @@ async def agentic_voice( ) except Exception as e: - from .handlers.message import _format_error_message - await progress_msg.edit_text(_format_error_message(e), parse_mode="HTML") logger.error( "Claude voice processing failed", error=str(e), user_id=user_id @@ -1552,8 +1542,6 @@ async def _handle_agentic_media_message( context.user_data["claude_session_id"] = claude_response.session_id - from .handlers.message import _update_working_directory_from_claude_response - _update_working_directory_from_claude_response( claude_response, context, self.settings, user_id ) diff --git a/src/bot/utils/error_messages.py b/src/bot/utils/error_messages.py new file mode 100644 index 000000000..719724838 --- /dev/null +++ b/src/bot/utils/error_messages.py @@ -0,0 +1,213 @@ +"""Render Claude errors as user-facing Telegram messages. + +Shared by agentic mode (``src/bot/orchestrator.py``) and classic mode +(``src/bot/handlers/message.py``). Lives here so that agentic mode does not +import from the classic handlers package. +""" + +from ...claude.exceptions import ( + ClaudeError, + ClaudeMCPError, + ClaudeParsingError, + ClaudeProcessError, + ClaudeSessionError, + ClaudeTimeoutError, +) +from .html_format import escape_html + + +def _format_error_message(error: Exception | str) -> str: + """Format error messages for user-friendly display. + + Accepts an exception object (preferred) or a string for backward + compatibility. When an exception is provided, the error type is used + to produce a specific, actionable message. + """ + # Normalise: keep both the object and a string representation. + if isinstance(error, str): + error_str = error + error_obj: Exception | None = None + else: + error_str = str(error) + error_obj = error + + # --- Dispatch on exception type first (most specific) --- + + if isinstance(error_obj, ClaudeTimeoutError): + return ( + "⏰ Request Timeout\n\n" + f"{escape_html(error_str)}\n\n" + "What you can do:\n" + "• Try breaking your request into smaller parts\n" + "• Avoid asking for very large file operations in one go\n" + "• Try again — transient slowdowns happen" + ) + + if isinstance(error_obj, ClaudeMCPError): + server_hint = "" + if error_obj.server_name: + server_hint = f" ({escape_html(error_obj.server_name)})" + return ( + f"🔌 MCP Server Error{server_hint}\n\n" + f"{escape_html(error_str)}\n\n" + "What you can do:\n" + "• Check that the MCP server is running and reachable\n" + "• Verify MCP_CONFIG_PATH points to a valid config\n" + "• Ask the administrator to check MCP server logs" + ) + + if isinstance(error_obj, ClaudeParsingError): + return ( + "📄 Response Parsing Error\n\n" + f"Claude returned a response that could not be parsed:\n" + f"{escape_html(error_str[:300])}\n\n" + "What you can do:\n" + "• Try your request again\n" + "• Rephrase your prompt if the problem persists" + ) + + if isinstance(error_obj, ClaudeSessionError): + return ( + "🔄 Session Error\n\n" + f"{escape_html(error_str)}\n\n" + "What you can do:\n" + "• Use /new to start a fresh session\n" + "• Try your request again\n" + "• Use /status to check your current session" + ) + + if isinstance(error_obj, ClaudeProcessError): + return _format_process_error(error_str) + + # Any future ClaudeError subtypes not explicitly handled above — + # preserve their existing message as-is rather than downgrading + # to a generic "process error". + if isinstance(error_obj, ClaudeError): + safe_error = escape_html(error_str) + if len(safe_error) > 500: + safe_error = safe_error[:500] + "..." + return ( + f"❌ Claude Error\n\n" + f"{safe_error}\n\n" + f"Try again or use /new to start a fresh session." + ) + + # --- Fall back to keyword matching (for string-only callers) -------- + # These patterns match the known error prefixes produced by + # sdk_integration.py and facade.py, NOT arbitrary user content. + + error_lower = error_str.lower() + + if "usage limit reached" in error_lower or "usage limit" in error_lower: + return error_str # Already user-friendly + + if "tool not allowed" in error_lower: + return error_str # Already formatted by facade.py + + if "no conversation found" in error_lower: + return ( + "🔄 Session Not Found\n\n" + "The previous Claude session could not be found or has expired.\n\n" + "What you can do:\n" + "• Use /new to start a fresh session\n" + "• Try your request again\n" + "• Use /status to check your current session" + ) + + if "rate limit" in error_lower: + return ( + "⏱️ Rate Limit Reached\n\n" + "Too many requests in a short time period.\n\n" + "What you can do:\n" + "• Wait a moment before trying again\n" + "• Use simpler requests\n" + "• Check your current usage with /status" + ) + + if "timed out after" in error_lower or "claude sdk timed out" in error_lower: + return ( + "⏰ Request Timeout\n\n" + f"{escape_html(error_str)}\n\n" + "What you can do:\n" + "• Try breaking your request into smaller parts\n" + "• Avoid asking for very large file operations in one go\n" + "• Try again — transient slowdowns happen" + ) + + if "overloaded" in error_lower: + return ( + "🏗️ Claude is Overloaded\n\n" + "The Claude API is currently experiencing high demand.\n\n" + "What you can do:\n" + "• Wait a moment and try again\n" + "• Shorter prompts may succeed more easily" + ) + + if "invalid api key" in error_lower or "authentication_error" in error_lower: + return ( + "🔑 API Authentication Error\n\n" + "The API key used to connect to Claude is invalid or expired.\n\n" + "What you can do:\n" + "• Ask the administrator to verify the " + "ANTHROPIC_API_KEY setting\n" + "• Check that the API key has not been revoked" + ) + + # Match known SDK prefixes: "Failed to connect to Claude: ..." + # and "MCP server connection failed: ..." + if error_lower.startswith("failed to connect to claude"): + return ( + "🌐 Connection Error\n\n" + f"Could not connect to Claude:\n" + f"{escape_html(error_str[:300])}\n\n" + "What you can do:\n" + "• Check your network / firewall settings\n" + "• Verify the Claude CLI is installed and accessible\n" + "• Try again in a moment" + ) + + # Match known SDK prefix: "Claude Code not found. ..." + if error_lower.startswith("claude code not found"): + return ( + "🔍 Claude CLI Not Found\n\n" + f"{escape_html(error_str)}\n\n" + "What you can do:\n" + "• Ensure Claude Code is installed: " + "npm install -g @anthropic-ai/claude-code\n" + "• Set the CLAUDE_CLI_PATH environment variable" + ) + + # Match known SDK prefixes: "MCP server error: ..." and + # "MCP server connection failed: ..." + if error_lower.startswith("mcp server"): + return ( + "🔌 MCP Server Error\n\n" + f"{escape_html(error_str)}\n\n" + "What you can do:\n" + "• Check that the MCP server is running\n" + "• Verify MCP configuration\n" + "• Ask the administrator to check MCP server logs" + ) + + # --- No match — show the raw error as-is --- + safe_error = escape_html(error_str) + if len(safe_error) > 500: + safe_error = safe_error[:500] + "..." + + return f"❌ {safe_error}" + + +def _format_process_error(error_str: str) -> str: + """Format a Claude process/SDK error with the actual details.""" + safe_error = escape_html(error_str) + if len(safe_error) > 500: + safe_error = safe_error[:500] + "..." + + return ( + f"❌ Claude Process Error\n\n" + f"{safe_error}\n\n" + "What you can do:\n" + "• Try your request again\n" + "• Use /new to start a fresh session if the problem persists\n" + "• Check /status for current session state" + ) diff --git a/src/bot/utils/working_directory.py b/src/bot/utils/working_directory.py new file mode 100644 index 000000000..a530ed248 --- /dev/null +++ b/src/bot/utils/working_directory.py @@ -0,0 +1,70 @@ +"""Track working-directory changes announced in Claude's replies. + +Shared by agentic mode (``src/bot/orchestrator.py``) and classic mode +(``src/bot/handlers/message.py``). Lives here so that agentic mode does not +import from the classic handlers package. +""" + +import re +from pathlib import Path + +import structlog + +logger = structlog.get_logger() + + +def _update_working_directory_from_claude_response( + claude_response, context, settings, user_id +): + """Update the working directory based on Claude's response content.""" + # Look for directory changes in Claude's response + # This searches for common patterns that indicate directory changes + patterns = [ + r"(?:^|\n).*?cd\s+([^\s\n]+)", # cd command + r"(?:^|\n).*?Changed directory to:?\s*([^\s\n]+)", # explicit directory change + r"(?:^|\n).*?Current directory:?\s*([^\s\n]+)", # current directory indication + r"(?:^|\n).*?Working directory:?\s*([^\s\n]+)", # working directory indication + ] + + content = claude_response.content.lower() + current_dir = context.user_data.get( + "current_directory", settings.approved_directory + ) + + for pattern in patterns: + matches = re.findall(pattern, content, re.MULTILINE | re.IGNORECASE) + for match in matches: + try: + # Clean up the path + new_path = match.strip().strip("\"'`") + + # Handle relative paths + if new_path.startswith("./") or new_path.startswith("../"): + new_path = (current_dir / new_path).resolve() + elif not new_path.startswith("/"): + # Relative path without ./ + new_path = (current_dir / new_path).resolve() + else: + # Absolute path + new_path = Path(new_path).resolve() + + # Validate that the new path is within the approved directory + if ( + new_path.is_relative_to(settings.approved_directory) + and new_path.exists() + ): + context.user_data["current_directory"] = new_path + logger.info( + "Updated working directory from Claude response", + old_dir=str(current_dir), + new_dir=str(new_path), + user_id=user_id, + ) + return # Take the first valid match + + except (ValueError, OSError) as e: + # Invalid path, skip this match + logger.debug( + "Invalid path in Claude response", path=match, error=str(e) + ) + continue diff --git a/tests/unit/test_bot/test_agentic_imports.py b/tests/unit/test_bot/test_agentic_imports.py new file mode 100644 index 000000000..4e441e792 --- /dev/null +++ b/tests/unit/test_bot/test_agentic_imports.py @@ -0,0 +1,60 @@ +"""Guard: agentic mode must not import from the classic handlers package. + +``src/bot/handlers/`` is classic-mode code slated for removal. Everything the +agentic orchestrator needs from it has been moved to ``src/bot/commands.py`` +and ``src/bot/utils/``; the one remaining import is the classic registration +inside ``_register_classic_handlers``. This test walks the orchestrator's AST +so a new ``from .handlers`` import cannot slip back in unnoticed. +""" + +import ast +from pathlib import Path + +ORCHESTRATOR = Path(__file__).resolve().parents[3] / "src" / "bot" / "orchestrator.py" + + +def _handler_imports(tree: ast.Module): + """Yield (import node, enclosing function name or None) for handlers imports.""" + for node in ast.walk(tree): + if not isinstance(node, ast.FunctionDef): + continue + for child in ast.walk(node): + if isinstance(child, ast.ImportFrom) and _targets_handlers(child): + yield child, node.name + for node in tree.body: + if isinstance(node, ast.ImportFrom) and _targets_handlers(node): + yield node, None + + +def _targets_handlers(node: ast.ImportFrom) -> bool: + module = node.module or "" + return module == "handlers" or module.startswith("handlers.") + + +def test_orchestrator_imports_handlers_only_for_classic_registration(): + tree = ast.parse(ORCHESTRATOR.read_text(encoding="utf-8")) + found = [(scope, ast.unparse(node)) for node, scope in _handler_imports(tree)] + assert found == [ + ( + "_register_classic_handlers", + "from .handlers import callback, command, message", + ) + ], f"unexpected classic-handler imports in orchestrator.py: {found}" + + +def test_shared_code_does_not_import_handlers(): + """The modules agentic mode imports must not reach back into handlers/.""" + shared = [ + Path("src/bot/commands.py"), + Path("src/bot/utils/error_messages.py"), + Path("src/bot/utils/working_directory.py"), + ] + root = ORCHESTRATOR.parents[2] + for rel in shared: + tree = ast.parse((root / rel).read_text(encoding="utf-8")) + bad = [ + ast.unparse(n) + for n in ast.walk(tree) + if isinstance(n, ast.ImportFrom) and "handlers" in (n.module or "") + ] + assert not bad, f"{rel} imports classic handlers: {bad}" diff --git a/tests/unit/test_bot/test_stop_button.py b/tests/unit/test_bot/test_stop_button.py index bb167d67a..c6ac3ed63 100644 --- a/tests/unit/test_bot/test_stop_button.py +++ b/tests/unit/test_bot/test_stop_button.py @@ -217,7 +217,7 @@ async def test_progress_message_has_stop_button(self, orchestrator, settings): mock_hb.return_value = mock_task with patch( - "src.bot.handlers.message._update_working_directory_from_claude_response" + "src.bot.orchestrator._update_working_directory_from_claude_response" ): with patch("src.bot.utils.formatting.ResponseFormatter") as MockFmt: MockFmt.return_value.format_claude_response.return_value = [] @@ -279,7 +279,7 @@ async def test_active_request_cleaned_up_after_success( mock_task.cancel = MagicMock() mock_hb.return_value = mock_task with patch( - "src.bot.handlers.message._update_working_directory_from_claude_response" + "src.bot.orchestrator._update_working_directory_from_claude_response" ): with patch("src.bot.utils.formatting.ResponseFormatter") as MockFmt: MockFmt.return_value.format_claude_response.return_value = [] @@ -324,7 +324,7 @@ async def test_active_request_cleaned_up_after_error(self, orchestrator, setting mock_task.cancel = MagicMock() mock_hb.return_value = mock_task with patch( - "src.bot.handlers.message._format_error_message", return_value="err" + "src.bot.orchestrator._format_error_message", return_value="err" ): await orchestrator.agentic_text(update, context) diff --git a/tests/unit/test_bot/test_thread_mode_handlers.py b/tests/unit/test_bot/test_thread_mode_handlers.py index 4ef8db30d..a2b8e2496 100644 --- a/tests/unit/test_bot/test_thread_mode_handlers.py +++ b/tests/unit/test_bot/test_thread_mode_handlers.py @@ -5,6 +5,8 @@ import pytest +from src.bot import commands as shared_commands +from src.bot.commands import sync_threads from src.bot.handlers import callback, command from src.config import create_test_config @@ -155,7 +157,7 @@ async def test_sync_threads_private_mode_rejects_non_private_chat(thread_setting } context.user_data = {} - await command.sync_threads(update, context) + await sync_threads(update, context) manager.sync_topics.assert_not_called() status_msg.edit_text.assert_called_once() @@ -180,7 +182,7 @@ async def test_sync_threads_reloads_registry_from_yaml(thread_settings, monkeypa new_registry = MagicMock() load_mock = MagicMock(return_value=new_registry) - monkeypatch.setattr(command, "load_project_registry", load_mock) + monkeypatch.setattr(shared_commands, "load_project_registry", load_mock) status_msg = AsyncMock() status_msg.edit_text = AsyncMock() @@ -201,7 +203,7 @@ async def test_sync_threads_reloads_registry_from_yaml(thread_settings, monkeypa } context.user_data = {} - await command.sync_threads(update, context) + await sync_threads(update, context) load_mock.assert_called_once_with( config_path=settings.projects_config_path, @@ -256,7 +258,7 @@ async def test_sync_threads_group_mode_rejects_non_target_chat(tmp_path: Path): } context.user_data = {} - await command.sync_threads(update, context) + await sync_threads(update, context) manager.sync_topics.assert_not_called() status_msg.edit_text.assert_called_once() diff --git a/tests/unit/test_orchestrator.py b/tests/unit/test_orchestrator.py index 108e0f808..bb006d08a 100644 --- a/tests/unit/test_orchestrator.py +++ b/tests/unit/test_orchestrator.py @@ -182,7 +182,7 @@ async def test_restart_command_sends_sigterm(deps): """restart_command sends SIGTERM to the current process.""" from unittest.mock import patch - from src.bot.handlers.command import restart_command + from src.bot.commands import restart_command update = MagicMock() update.effective_user.id = 123 @@ -191,7 +191,7 @@ async def test_restart_command_sends_sigterm(deps): context = MagicMock() context.bot_data = {"audit_logger": None} - with patch("src.bot.handlers.command.os.kill") as mock_kill: + with patch("src.bot.commands.os.kill") as mock_kill: await restart_command(update, context) import os