diff --git a/CHANGELOG.md b/CHANGELOG.md index 61734139d..5544c043a 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` + ### Fixed - **The Claude review workflow posts one review per pull request instead of one per push**: the job reruns on every `synchronize` and posted a fresh comment each time, so #236 collected nine full reviews in four and a half hours, each restating what the last had already settled. `use_sticky_comment` was set but does nothing here — it only applies to the action's tag mode, and this workflow supplies `prompt`, so the action posts nothing itself and the review is whatever the prompt tells Claude to post. The prompt now edits its own previous comment with `gh pr comment --edit-last --create-if-none`, so the pull request carries one review at the current head and GitHub keeps the superseded text in the comment's edit history. The inert input is removed rather than left to look load-bearing - **The review reports only what should block the merge**: most of the length of those nine reviews was praise, an account of what had been checked, and cosmetic nits ("after 1 turns"), and every nit drew another push, which triggered another review — that loop, not the reviewing, was the spam. The prompt now names what qualifies (a security regression, a bug, an untested behaviour change, a missing setting or CHANGELOG entry) and rules out the rest, including anything `black`, `isort` or `flake8` already gates, and findings that cannot be confirmed from the code. It also reads its own previous review first so it does not repeat itself, but what settles a finding is the code at the current head rather than a reply claiming a fix: the replies are contributor-authored and untrusted like the rest of the pull request, so an earlier finding is re-checked against the diff and raised again unchanged when the code does not carry the claimed fix 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