diff --git a/cecli/args.py b/cecli/args.py index a9dbab5114c..317523944fe 100644 --- a/cecli/args.py +++ b/cecli/args.py @@ -132,6 +132,15 @@ def get_parser(default_config_files, git_root): "Specify a file with model tag overrides (e.g., gpt-4o:high -> reasoning_effort: high)" ), ).complete = shtab.FILE + group.add_argument( + "--model-providers", + metavar="MODEL_PROVIDERS_JSON", + help=( + "Specify custom OpenAI-compatible model providers as a JSON/YAML string (e.g.," + ' \'{"my-provider": {"api_base": "https://...", "api_key_env": ["MY_KEY"]}}\')' + ), + default=None, + ) group.add_argument( "--reasoning-effort", type=str, @@ -1113,7 +1122,7 @@ def get_parser(default_config_files, git_root): group.add_argument( "--notification-bell", action=argparse.BooleanOptionalAction, - default=True, + default=False, help=( "Allow notification commands to produce an audible bell. When enabled, command" " output is not suppressed so terminal bell escape sequences can ring through" diff --git a/cecli/change_tracker.py b/cecli/change_tracker.py index 961471b32ec..a07162c1e7f 100644 --- a/cecli/change_tracker.py +++ b/cecli/change_tracker.py @@ -21,7 +21,7 @@ def track_change( Parameters: - file_path: Path to the file that was changed - - change_type: Type of change (e.g., 'edittext', 'insertlines') + - change_type: Type of change (e.g., 'editfile', 'insertlines') - original_content: Original content before the change - new_content: New content after the change - metadata: Additional information about the change (line numbers, positions, etc.) diff --git a/cecli/coders/agent_coder.py b/cecli/coders/agent_coder.py index 7366d2f859e..64a87143331 100644 --- a/cecli/coders/agent_coder.py +++ b/cecli/coders/agent_coder.py @@ -65,7 +65,7 @@ def __init__(self, *args, **kwargs): "commandinteractive", "explorecode", "ls", - "readrange", + "readfile", "grep", "thinking", "updatetodolist", @@ -73,7 +73,7 @@ def __init__(self, *args, **kwargs): self.write_tools = { "command", "commandinteractive", - "edittext", + "editfile", "undochange", } self.edit_allowed = True @@ -165,6 +165,7 @@ def _get_agent_config(self): config["command_timeout"] = nested.getter(config, "command_timeout", 30) config["allowed_commands"] = nested.getter(config, "allowed_commands", []) config["hot_reload"] = nested.getter(config, "hot_reload", False) + config["diff_colors"] = nested.getter(config, "diff_colors", True) config["allow_nested_delegation"] = nested.getter(config, "allow_nested_delegation", False) config["tools_paths"] = nested.getter(config, ["tools_paths", "tool_paths"], []) @@ -1138,7 +1139,7 @@ def _generate_tool_context(self, repetitive_tools): context_parts.append("\n\n") context_parts.append("## File Editing Tools Disabled") context_parts.append( - "File editing tools are currently disabled. Use `ReadRange` to determine the" + "File editing tools are currently disabled. Use `ReadFile` to determine the" " current content ID prefixes needed to perform an edit and activate them when" " you are ready to edit a file." ) diff --git a/cecli/commands/add.py b/cecli/commands/add.py index c4a4e31d15b..98291353488 100644 --- a/cecli/commands/add.py +++ b/cecli/commands/add.py @@ -125,10 +125,11 @@ async def execute(cls, io, coder, args, **kwargs): else: io.tool_error(f"Cannot add {matched_file} as it's not part of the repository") else: - if is_image_file(matched_file) and not coder.main_model.info.get("supports_vision"): + active_model = coder.get_active_model() + if is_image_file(matched_file) and not active_model.info.get("supports_vision"): io.tool_error( f"Cannot add image file {matched_file} as the" - f" {coder.main_model.name} does not support images." + f" {active_model.name} does not support images." ) continue content = io.read_text(abs_file_path) diff --git a/cecli/commands/clear.py b/cecli/commands/clear.py index 793b55d80ee..e8be1ff8b54 100644 --- a/cecli/commands/clear.py +++ b/cecli/commands/clear.py @@ -25,10 +25,9 @@ async def execute(cls, io, coder, args, **kwargs): # Clear TUI output if available if coder.tui and coder.tui(): - coder.tui().action_clear_output() + coder.tui().call_later(coder.tui().action_clear_output) - io.tool_output("All chat history cleared.") - return format_command_result(io, "clear", "Cleared chat history") + return format_command_result(io, "clear", "All chat history cleared.") @classmethod def get_completions(cls, io, coder, args) -> List[str]: diff --git a/cecli/commands/read_only.py b/cecli/commands/read_only.py index b5f5b4d98d9..ff5f8cad6e6 100644 --- a/cecli/commands/read_only.py +++ b/cecli/commands/read_only.py @@ -156,10 +156,11 @@ def _add_read_only_file( source_mode="read-only", target_mode="read-only", ): - if is_image_file(original_name) and not coder.main_model.info.get("supports_vision"): + active_model = coder.get_active_model() + if is_image_file(original_name) and not active_model.info.get("supports_vision"): io.tool_error( f"Cannot add image file {original_name} as the" - f" {coder.main_model.name} does not support images." + f" {active_model.name} does not support images." ) return diff --git a/cecli/commands/read_only_stub.py b/cecli/commands/read_only_stub.py index c75ff9b6628..240fd8a25c9 100644 --- a/cecli/commands/read_only_stub.py +++ b/cecli/commands/read_only_stub.py @@ -156,10 +156,11 @@ def _add_read_only_file( source_mode="read-only", target_mode="read-only", ): - if is_image_file(original_name) and not coder.main_model.info.get("supports_vision"): + active_model = coder.get_active_model() + if is_image_file(original_name) and not active_model.info.get("supports_vision"): io.tool_error( f"Cannot add image file {original_name} as the" - f" {coder.main_model.name} does not support images." + f" {active_model.name} does not support images." ) return diff --git a/cecli/commands/reset.py b/cecli/commands/reset.py index 78841e3c1fa..0a4db1dc425 100644 --- a/cecli/commands/reset.py +++ b/cecli/commands/reset.py @@ -29,7 +29,7 @@ async def execute(cls, io, coder, args, **kwargs): # Clear TUI output if available if coder.tui and coder.tui(): - coder.tui().action_clear_output() + coder.tui().call_later(coder.tui().action_clear_output) else: io.tool_output("All files dropped and chat history cleared.") diff --git a/cecli/helpers/agents/service.py b/cecli/helpers/agents/service.py index 16369dfbad6..273b7e1e4b9 100644 --- a/cecli/helpers/agents/service.py +++ b/cecli/helpers/agents/service.py @@ -191,8 +191,13 @@ def build_registry(cls, paths: List[str]) -> None: from .config import parse_subagent_file + # Always check the default sub-agents directory in the user's home + default_dir = str(Path.home() / ".cecli" / "subagents") + if default_dir not in paths: + paths = [default_dir] + list(paths) + for directory in paths: - dir_path = Path(directory) + dir_path = Path(directory).expanduser() if not dir_path.is_dir(): continue for md_file in sorted(dir_path.glob("*.md")): diff --git a/cecli/helpers/conversation/files.py b/cecli/helpers/conversation/files.py index ca239aeb92f..f28fd0f7ef9 100644 --- a/cecli/helpers/conversation/files.py +++ b/cecli/helpers/conversation/files.py @@ -252,6 +252,7 @@ def generate_diff(self, fname: str) -> Optional[str]: ) if not snapshot_content: + self.add_file(abs_fname, content=current_content) return None # Generate diff between snapshot and current content using hashline helper diff --git a/cecli/helpers/conversation/integration.py b/cecli/helpers/conversation/integration.py index 464023f6a3e..51a130d482f 100644 --- a/cecli/helpers/conversation/integration.py +++ b/cecli/helpers/conversation/integration.py @@ -1092,21 +1092,21 @@ def add_copy_paste_tool_instructions(self) -> None: " param_value_json\n" " \n\n" " Example:\n" - " \n" + " \n" ' [{"file_path": "example.py", "range_start": "def hello",' ' "range_end": "def goodbye"}]\n' " \n\n" "2. JSON Tool-Call Format:\n" ' Embed a JSON object with "name" and "arguments" keys.\n\n' " Example:\n" - ' {"name": "Local--ReadRange", "arguments": {"read": [{"file_path": "example.py",' + ' {"name": "Local--ReadFile", "arguments": {"read": [{"file_path": "example.py",' ' "range_start": "class A", "range_end": "class Z"}]}}\n\n' "3. Bracket Format:\n" " [ToolName(key1=value1, key2=value2)]\n\n" " Example:\n" - ' [Local--ReadRange(read=[{"file_path": "example.py", "range_start": "class A"}])]\n\n' + ' [Local--ReadFile(read=[{"file_path": "example.py", "range_start": "class A"}])]\n\n' "IMPORTANT: Use the FULL prefixed tool name " - '(e.g., "Local--ReadRange" not just "ReadRange").\n' + '(e.g., "Local--ReadFile" not just "ReadFile").\n' "" ) diff --git a/cecli/helpers/hashline.py b/cecli/helpers/hashline.py index e886558f1d8..0db146d7100 100644 --- a/cecli/helpers/hashline.py +++ b/cecli/helpers/hashline.py @@ -317,7 +317,7 @@ def _looks_like_content_id(value: str) -> bool: def _find_substring_matches(lines, value): """Find all line indices where the value appears as a substring.""" - value_stripped = value.rstrip("\r\n") + value_stripped = value.strip() return [i for i, line in enumerate(lines) if value_stripped in line] def _resolve_to_hash_id(lines, idx, hp): @@ -409,6 +409,7 @@ def get_hashline_diff( end_line_hash, operation, text=None, + pretty=False, ): """ Generate a diff for a hashline operation in the format used by the original format_output. @@ -420,6 +421,8 @@ def get_hashline_diff( end_line_hash: Hashline format for end line: "{4 char hash}" (without the braces) operation: One of "replace", "insert", or "delete" text: Text to insert or replace with (required for replace/insert operations) + pretty: When True, color removed lines in magenta (\x1b[95m) and added lines + in light green (\033[32m). When False, all diff lines use \033[92m. Returns: str: A formatted diff snippet showing changes, or empty string if no changes @@ -523,7 +526,18 @@ def get_hashline_diff( diff_lines = list(diff)[2:] if diff_lines: - return "\n".join([line for line in diff_lines]) + if pretty: + colored_lines = [] + for line in diff_lines: + if line.startswith("-"): + colored_lines.append(f"\033[38;5;96m{line}\x1b[0m") + elif line.startswith("+"): + colored_lines.append(f"\033[32m{line}\x1b[0m") + else: + colored_lines.append(line) + return "\n".join(colored_lines) + else: + return "\n".join([line for line in diff_lines]) else: return "" diff --git a/cecli/helpers/model_providers.py b/cecli/helpers/model_providers.py index d96f4163e2b..50fc45e9957 100644 --- a/cecli/helpers/model_providers.py +++ b/cecli/helpers/model_providers.py @@ -289,6 +289,16 @@ def __init__(self, provider_configs: Optional[Dict[str, Dict]] = None) -> None: def set_verify_ssl(self, verify_ssl: bool) -> None: self.verify_ssl = verify_ssl + def merge_provider_configs(self, user_configs: Dict[str, Dict]) -> None: + """Merge user-defined provider configs into the existing provider configs.""" + for slug, cfg in user_configs.items(): + if slug in self.provider_configs: + self.provider_configs[slug] = _deep_merge(self.provider_configs[slug], cfg) + else: + self.provider_configs[slug] = deepcopy(cfg) + self._provider_cache[slug] = None + self._cache_loaded[slug] = False + def supports_provider(self, provider: Optional[str]) -> bool: return bool(provider and provider in self.provider_configs) @@ -571,6 +581,12 @@ def _get_account_id(self, provider: str) -> Optional[str]: return None +def register_user_providers_with_litellm(user_configs: Dict[str, Dict]) -> None: + """Register user-defined providers with LiteLLM for custom handler support.""" + for slug, cfg in user_configs.items(): + _register_provider_with_litellm(slug, cfg) + + def ensure_litellm_providers_registered() -> None: """One-time registration guard for LiteLLM provider metadata.""" global _PROVIDERS_REGISTERED diff --git a/cecli/helpers/responses.py b/cecli/helpers/responses.py index 8a77db27bb4..68d69330718 100644 --- a/cecli/helpers/responses.py +++ b/cecli/helpers/responses.py @@ -166,7 +166,7 @@ def extract_tools_from_pseudo_json(content: str) -> Optional[List[ChatCompletion The parser handles nested parentheses and commas inside JSON values. Example: - [Local--ReadRange(show=[{"file_path": "agent.py", "start_text": "class A"}], verbose=true, mode="strict")] + [Local--ReadFile(show=[{"file_path": "agent.py", "start_text": "class A"}], verbose=true, mode="strict")] """ from litellm.types.utils import ChatCompletionMessageToolCall, Function # noqa diff --git a/cecli/helpers/skills.py b/cecli/helpers/skills.py index e5331de7ba8..2d1667a2bf8 100644 --- a/cecli/helpers/skills.py +++ b/cecli/helpers/skills.py @@ -66,6 +66,11 @@ def __init__( git_root: Optional git root directory for relative path resolution coder: Optional reference to the coder instance (weak reference) """ + # Always include the default skills directory in the user's home + default_skill_dir = str(Path.home() / ".cecli" / "skills") + if default_skill_dir not in directory_paths: + directory_paths = [default_skill_dir] + list(directory_paths) + self.directory_paths = [Path(p).expanduser().resolve() for p in directory_paths] self.include_list = set(include_list) if include_list else None self.exclude_list = set(exclude_list) if exclude_list else set() diff --git a/cecli/hooks/__init__.py b/cecli/hooks/__init__.py index bacd9fdd354..ae1c4409e81 100644 --- a/cecli/hooks/__init__.py +++ b/cecli/hooks/__init__.py @@ -1,6 +1,7 @@ """Hooks module for extending cecli functionality.""" from .base import BaseHook, CommandHook +from .helpers import HookHelpers from .integration import HookIntegration from .manager import HookManager from .registry import HookRegistry @@ -10,6 +11,7 @@ __all__ = [ "BaseHook", "CommandHook", + "HookHelpers", "HookIntegration", "HookManager", "HookRegistry", diff --git a/cecli/hooks/helpers.py b/cecli/hooks/helpers.py new file mode 100644 index 00000000000..7b8ce539c1a --- /dev/null +++ b/cecli/hooks/helpers.py @@ -0,0 +1,188 @@ +"""Helpers for writing Python hooks. + +Provides a higher-level API for accessing conversation history, making model +calls, and working with sub-agents from within hook implementations. + +Typical usage:: + + from cecli.hooks import HookHelpers + + class MyHook(BaseHook): + type = HookType.POST_TOOL + + async def execute(self, coder, metadata): + recent = HookHelpers.get_messages(coder, last_n=5) + reply = await HookHelpers.call(coder, prompt="Summarize the above.") + HookHelpers.append_message(coder, {"role": "assistant", "content": reply}) + summary = await HookHelpers.call_subagent( + coder, "reviewer", "Review these changes" + ) + return True +""" + +from typing import Any, Dict, List, Optional + + +class HookHelpers: + """Collection of static helper methods for Python hooks. + + All methods receive the ``coder`` instance as their first argument + so they can access conversation history, make model calls, and + invoke sub-agents on behalf of the agent running the hook. + """ + + @staticmethod + def get_messages( + coder: Any, + last_n: Optional[int] = None, + tag: Optional[str] = None, + reload: bool = False, + ) -> List[Dict[str, Any]]: + """Retrieve conversation messages for the given coder. + + This is a convenience wrapper around + ``ConversationService.get_manager(coder).get_messages_dict()``. + + Args: + coder: The coder instance (passed to hook's ``execute()``). + last_n: If set, return only the *last_n* messages (most recent first). + tag: Optional tag to filter by (e.g. ``"cur"``, ``"done"``). + If ``None``, returns all messages. + reload: If ``True``, bypass the internal cache. + + Returns: + A list of message dicts sorted by priority then timestamp, + each with keys ``role``, ``content``, etc. + """ + from cecli.helpers.conversation.service import ConversationService + + manager = ConversationService.get_manager(coder) + messages = manager.get_messages_dict(tag=tag, reload=reload) + + if last_n is not None and last_n > 0: + messages = messages[-last_n:] + + return messages + + @staticmethod + def append_message( + coder: Any, + message_dict: Dict[str, Any], + tag: str = "cur", + **kwargs: Any, + ) -> Any: + """Append a message to the coder's conversation history. + + This is a convenience wrapper around + ``ConversationService.get_manager(coder).add_message()``. + + Args: + coder: The coder instance (passed to hook's ``execute()``). + message_dict: The message content dict (e.g. + ``{"role": "user", "content": "..."}``). + tag: Message tag to use (default ``"cur"``). + **kwargs: Additional keyword arguments forwarded to + ``add_message()``, such as ``hash_key``, ``force``, + ``priority``, ``promotion``, etc. + + Returns: + The ``BaseMessage`` instance that was created or updated. + """ + from cecli.helpers.conversation.service import ConversationService + + return ConversationService.get_manager(coder).add_message( + message_dict=message_dict, + tag=tag, + **kwargs, + ) + + @staticmethod + async def call( + coder: Any, + messages: Optional[List[Dict[str, Any]]] = None, + prompt: Optional[str] = None, + system: Optional[str] = None, + model_name: Optional[str] = None, + max_tokens: Optional[int] = None, + **kwargs: Any, + ) -> Optional[str]: + """Make a language model generation call. + + You can provide ``messages`` directly (a list of message dicts), or + use ``prompt`` (with an optional ``system`` message) to build a + simple user/assistant exchange. + + Args: + coder: The coder instance (passed to hook's ``execute()``). + messages: A list of message dicts (``{"role": ..., "content": ...}``). + If provided, ``prompt`` and ``system`` are ignored. + prompt: A simple user prompt string. Ignored if ``messages`` is set. + system: An optional system prompt. Only used when ``prompt`` is set + and ``messages`` is ``None``. + model_name: Override the model to use (e.g. ``"gpt-4o"``). + If ``None``, uses ``coder.main_model``. + max_tokens: Maximum tokens for the response. + **kwargs: Additional keyword arguments forwarded to + ``Model.simple_send_with_retries()``. + + Returns: + The generated text content, or ``None`` on failure. + """ + from cecli.models import Model + + if messages is None: + msgs: List[Dict[str, Any]] = [] + if system: + msgs.append({"role": "system", "content": system}) + if prompt: + msgs.append({"role": "user", "content": prompt}) + messages = msgs + + if not messages: + return None + + if model_name: + model = Model( + model_name, + from_model=coder.main_model, + ) + else: + model = coder.main_model + + return await model.simple_send_with_retries( + messages=messages, + max_tokens=max_tokens, + coder=coder, + override_kwargs=kwargs, + ) + + @staticmethod + async def call_subagent( + coder: Any, + name: str, + prompt: str, + **kwargs: Any, + ) -> Optional[str]: + """Invoke a sub-agent by name with the given prompt (blocking). + + This is a convenience wrapper around + ``AgentService.get_instance(coder).invoke()``. + + Args: + coder: The coder instance (passed to hook's ``execute()``). + name: The registered name of the sub-agent (e.g. + ``"reviewer"``, ``"tester"``). + prompt: The user message to pass to the sub-agent. + **kwargs: Additional keyword arguments forwarded to + ``AgentService.invoke()``, such as ``blocking``, + ``parent``, ``auto_reap``. + + Returns: + The sub-agent's summary string, or ``None`` if it failed or + was invoked non-blocking. + """ + from cecli.helpers.agents.service import AgentService + + agent_service = AgentService.get_instance(coder) + + return await agent_service.invoke(name, prompt, **kwargs) diff --git a/cecli/io.py b/cecli/io.py index d7633a52579..04438faaad9 100644 --- a/cecli/io.py +++ b/cecli/io.py @@ -367,7 +367,7 @@ def __init__( root=".", notifications=False, notifications_command=None, - notification_bell=True, + notification_bell=False, verbose=False, ): self.console = Console() @@ -386,6 +386,7 @@ def __init__( self.bell_on_next_input = False self.notifications = notifications self.notification_bell = notification_bell + self.custom_notification_command = False self.verbose = verbose self.profile_start_time = None self.profile_last_time = None @@ -404,10 +405,19 @@ def __init__( self.confirmation_input_active = False self.saved_input_text = "" - if notifications and notifications_command is None: - self.notifications_command = self.get_default_notification_command() - else: + if notifications_command is not None: + # Custom notification command overrides everything self.notifications_command = notifications_command + self.custom_notification_command = True + elif notification_bell and not notifications: + # Bell only mode - no command needed, _send_notification will fallback to bell + self.notifications_command = None + elif notifications: + # Generate default notification command based on notification_bell flag + include_bell = notification_bell is not False + self.notifications_command = self.get_default_notification_command(bell=include_bell) + else: + self.notifications_command = None no_color = os.environ.get("NO_COLOR") if no_color is not None and no_color != "": @@ -1693,8 +1703,8 @@ def llm_started(self): """Mark that the LLM has started processing, so we should ring the bell on next input""" self.bell_on_next_input = True - def get_default_notification_command(self): - """Return a command that triggers a system bell followed by a notification.""" + def get_default_notification_command(self, bell=True): + """Return a command that triggers a notification, optionally with a bell.""" import platform import shutil @@ -1744,13 +1754,18 @@ def get_default_notification_command(self): ) notif_cmd = f"powershell -WindowStyle Hidden -Command {ps_body}" - # 3. Concatenate them + # 3. Concatenate them based on bell preference if notif_cmd: - # Using ';' as a command separator works for both shell=True on Unix and Windows - return f"{bell_cmd} ; {notif_cmd}" + if bell: + # Using ';' as a command separator works for both shell=True on Unix and Windows + return f"{bell_cmd} ; {notif_cmd}" + return notif_cmd # Fallback if no notification tool is found - return bell_cmd + if bell: + return bell_cmd + + return None def _send_notification(self): # Cooldown to prevent notification spam @@ -1787,18 +1802,33 @@ def _send_notification(self): except Exception as e: self.tool_warning(f"Failed to run notifications command: {e}") - else: - print("\a", end="", flush=True) # Ring the bell + elif self.notification_bell is not False: + bell_cmd = self.get_default_notification_command(bell=True) + if bell_cmd: + try: + kwargs = { + "shell": True, + } + + if platform.system() == "Windows": + kwargs["creationflags"] = subprocess.CREATE_NO_WINDOW + else: + kwargs["start_new_session"] = True + + subprocess.Popen(bell_cmd, **kwargs) + except Exception as e: + self.tool_warning(f"Failed to run bell command: {e}") def notify_user_input_required(self): """Send a notification that user input is required.""" - if self.notifications: + if self.notifications or self.notification_bell or self.custom_notification_command: self._send_notification() def ring_bell(self): """Ring the terminal bell if needed and clear the flag""" - if self.bell_on_next_input and self.notifications: - self._send_notification() + if self.bell_on_next_input: + if self.notifications or self.notification_bell or self.custom_notification_command: + self._send_notification() self.bell_on_next_input = False def toggle_multiline_mode(self): diff --git a/cecli/llm.py b/cecli/llm.py index 84add1f6ac2..c46a2e49d54 100644 --- a/cecli/llm.py +++ b/cecli/llm.py @@ -53,7 +53,11 @@ def _load_litellm(self): self._lazy_module.suppress_debug_info = True self._lazy_module.set_verbose = False self._lazy_module.drop_params = True - self._lazy_module._logging._disable_debugging() + try: + self._lazy_module._logging._disable_debugging() + except AttributeError: + # litellm >= 1.62.0 removed the _logging module + pass # Make sure JSON-based OpenAI-compatible providers are registered ensure_litellm_providers_registered() diff --git a/cecli/main.py b/cecli/main.py index 2cd40d7cc9b..aea0c6b8690 100644 --- a/cecli/main.py +++ b/cecli/main.py @@ -311,6 +311,12 @@ def load_dotenv_files(git_root, dotenv_fname, encoding="utf-8"): if oauth_keys_file.exists(): dotenv_files.insert(0, str(oauth_keys_file.resolve())) dotenv_files = list(dict.fromkeys(dotenv_files)) + + # Also check ~/.cecli/.env as a default dotenv file (lowest priority) + cecli_env_file = Path.home() / ".cecli" / ".env" + if cecli_env_file.exists(): + dotenv_files.insert(0, str(cecli_env_file.resolve())) + dotenv_files = list(dict.fromkeys(dotenv_files)) loaded = [] for fname in dotenv_files: try: @@ -554,6 +560,7 @@ async def main_async( git_root = get_git_root() conf_fname = handle_core_files(Path(".cecli.conf.yml")) default_config_files = [ + str(Path.home() / ".cecli" / "conf.yml"), str(Path.home() / ".cecli.conf.yml"), str(Path(".cecli.conf.yml")), ] @@ -622,6 +629,8 @@ async def main_async( args.hooks = convert_yaml_to_json_string(args.hooks) if hasattr(args, "workspaces") and args.workspaces is not None: args.workspaces = convert_yaml_to_json_string(args.workspaces) + if hasattr(args, "model_providers") and args.model_providers is not None: + args.model_providers = convert_yaml_to_json_string(args.model_providers) # Interpolate environment variables in all string arguments for key, value in vars(args).items(): @@ -855,6 +864,23 @@ def get_io(pretty): await check_and_load_imports(io, is_first_run, verbose=args.verbose) register_models(git_root, args.model_settings_file, io, verbose=args.verbose) register_litellm_models(git_root, args.model_metadata_file, io, verbose=args.verbose) + if args.model_providers: + try: + user_providers = json.loads(args.model_providers) + if isinstance(user_providers, dict): + models.model_info_manager.provider_manager.merge_provider_configs(user_providers) + from cecli.helpers.model_providers import ( + register_user_providers_with_litellm, + ) + + register_user_providers_with_litellm(user_providers) + if args.verbose: + io.tool_output(f"Loaded {len(user_providers)} custom model provider(s):") + for slug in user_providers: + io.tool_output(f" - {slug}") + except json.JSONDecodeError as e: + io.tool_error(f"Failed to parse --model-providers JSON: {e}") + if args.list_models: models.print_matching_models(io, args.list_models) return await graceful_exit(None) diff --git a/cecli/mcp/manager.py b/cecli/mcp/manager.py index 4ee86d2496f..2c9246b1424 100644 --- a/cecli/mcp/manager.py +++ b/cecli/mcp/manager.py @@ -48,6 +48,36 @@ def _log_warning(self, message: str) -> None: if self.io: self.io.tool_warning(message) + @staticmethod + def _validate_server_config(config: dict) -> dict: + """ + Validate keepalive_interval in the server configuration. + + Args: + config: Server configuration dictionary + + Returns: + The validated configuration dictionary + + Raises: + ValueError: If keepalive_interval is invalid + """ + keepalive_interval = config.get("keepalive_interval") + + if keepalive_interval is not None: + if not isinstance(keepalive_interval, int) or isinstance(keepalive_interval, bool): + raise ValueError( + f"keepalive_interval must be an integer, got {type(keepalive_interval).__name__}" + ) + + if keepalive_interval < 5: + raise ValueError(f"keepalive_interval {keepalive_interval} is below minimum of 5") + + if keepalive_interval > 300: + raise ValueError(f"keepalive_interval {keepalive_interval} is above maximum of 300") + + return config + @property def servers(self) -> list["McpServer"]: """Get the list of managed MCP servers.""" diff --git a/cecli/mcp/server.py b/cecli/mcp/server.py index f148e47bd87..bb92c1473dd 100644 --- a/cecli/mcp/server.py +++ b/cecli/mcp/server.py @@ -1,8 +1,10 @@ import asyncio import logging import os +import random import webbrowser from contextlib import AsyncExitStack +from enum import Enum, auto from urllib.parse import urlparse import httpx @@ -20,6 +22,18 @@ save_mcp_oauth_token, ) +MIN_KEEPALIVE_INTERVAL = 5 +MAX_KEEPALIVE_INTERVAL = 300 +FAILED_PING_THRESHOLD = 3 + +logger = logging.getLogger(__name__) + + +class ConnectionState(Enum): + CONNECTED = auto() + UNHEALTHY = auto() + DISCONNECTED = auto() + class McpServer: """ @@ -120,6 +134,13 @@ async def disconnect(self): class HttpBasedMcpServer(McpServer): """Base class for HTTP-based MCP servers (HTTP streaming and SSE).""" + def __init__(self, server_config, io=None, verbose=False): + super().__init__(server_config, io, verbose) + self._state: ConnectionState = ConnectionState.CONNECTED + self._failed_pings: int = 0 + self._keepalive_task: asyncio.Task | None = None + self._http_client: httpx.AsyncClient | None = None + async def _create_oauth_provider(self): """Create an OAuthClientProvider using the MCP SDK.""" parsed = urlparse(self.config.get("url")) @@ -228,6 +249,7 @@ async def connect(self): timeout=30, ) ) + self._http_client = http_client transport = await self.exit_stack.enter_async_context( self._create_transport(url, http_client=http_client) @@ -238,9 +260,10 @@ async def connect(self): session = await self.exit_stack.enter_async_context(ClientSession(read, write)) await session.initialize() self.session = session + await self.start_keepalive() self._connection_loop = current_loop - if oauth_provider.context.oauth_metadata: + if oauth_provider is not None and oauth_provider.context.oauth_metadata: token_endpoint = oauth_provider._get_token_endpoint() server_info = get_mcp_oauth_token(self.name) if "client_info" not in server_info: @@ -249,17 +272,132 @@ async def connect(self): server_info["client_info"]["token_endpoint"] = token_endpoint save_mcp_oauth_token(self.name, server_info) - - return session except Exception as e: logging.error(f"Error initializing {self.name}: {e}") await self.disconnect() raise - async def disconnect(self): + async def start_keepalive(self): + """Start the background keepalive loop if configured.""" + interval = self.config.get("keepalive_interval") + if interval is None: + return + + try: + interval = int(interval) + if not (MIN_KEEPALIVE_INTERVAL <= interval <= MAX_KEEPALIVE_INTERVAL): + if self.verbose and self.io: + self.io.tool_warning( + f"Keepalive interval {interval} out of range ({MIN_KEEPALIVE_INTERVAL}-" + f"{MAX_KEEPALIVE_INTERVAL}). Ignoring." + ) + return + except (ValueError, TypeError): + if self.verbose and self.io: + self.io.tool_warning(f"Invalid keepalive interval {interval}. Must be an integer.") + return + + if self._keepalive_task and not self._keepalive_task.done(): + self._keepalive_task.cancel() + + self._keepalive_task = asyncio.create_task(self._keepalive_loop(interval)) + logger.info(f"Keepalive task started for {self.name} (interval: {interval}s)") + if self.verbose and self.io: + self.io.tool_output(f"Started keepalive loop for {self.name} (interval: {interval}s)") + + async def _keepalive_loop(self, interval: int): + """Background loop that sends periodic heartbeats to the MCP server.""" + try: + while True: + # Jitter: ±10% to prevent timing analysis + jitter = interval * 0.1 * (2 * random.random() - 1) + await asyncio.sleep(interval + jitter) + + if not self._http_client: + continue + + try: + url = self.config.get("url") + headers = self.config.get("headers", {}) + + # Use OPTIONS request as a lightweight heartbeat + response = await self._http_client.options(url, headers=headers) + if response.status_code == 200: + self._state = ConnectionState.CONNECTED + self._failed_pings = 0 + else: + raise httpx.HTTPStatusError( + f"Unexpected status {response.status_code}", + request=response.request, + response=response, + ) + except Exception: + self._failed_pings += 1 + if self._failed_pings >= FAILED_PING_THRESHOLD: + self._state = ConnectionState.DISCONNECTED + if self.verbose and self.io: + self.io.tool_warning( + f"MCP server {self.name} disconnected after {self._failed_pings} failed" + " pings. Attempting reconnect..." + ) + await self.reconnect() + else: + self._state = ConnectionState.UNHEALTHY + if self.verbose and self.io: + self.io.tool_output( + f"MCP server {self.name} unhealthy (ping {self._failed_pings}/{FAILED_PING_THRESHOLD})" + ) + except asyncio.CancelledError: + pass + except Exception as e: + logging.error(f"Keepalive loop for {self.name} crashed: {e}") + + async def reconnect(self): + """Attempt to reconnect to the server using exponential backoff.""" + initial_delay = 1 + multiplier = 2 + max_delay = 300 + + attempt = 0 + while self._state == ConnectionState.DISCONNECTED: + delay = min(initial_delay * (multiplier**attempt), max_delay) + # Jitter: ±20% + jitter = delay * 0.2 * (2 * random.random() - 1) + await asyncio.sleep(delay + jitter) + + try: + if self.verbose and self.io: + self.io.tool_output( + f"Attempting to reconnect to {self.name} (attempt {attempt + 1})..." + ) + + # Clean up old session/client without cancelling the keepalive task + await self.disconnect(cancel_keepalive=False) + await self.connect() + + self._state = ConnectionState.CONNECTED + self._failed_pings = 0 + if self.verbose and self.io: + self.io.tool_output(f"Successfully reconnected to {self.name}") + break + except Exception as e: + attempt += 1 + if self.verbose and self.io: + self.io.tool_warning( + f"Reconnection attempt {attempt} failed for {self.name}: {e}" + ) + + async def disconnect(self, cancel_keepalive: bool = True): """Disconnect from the MCP server and clean up resources.""" async with self._cleanup_lock: try: + if cancel_keepalive and self._keepalive_task: + self._keepalive_task.cancel() + try: + await asyncio.wait_for(self._keepalive_task, timeout=15) + except asyncio.CancelledError: + pass + logger.info(f"Keepalive task stopped for {self.name}") if hasattr(self, "_oauth_shutdown"): self._oauth_shutdown() await self.exit_stack.aclose() @@ -271,6 +409,7 @@ async def disconnect(self): logging.error(f"Error during cleanup of server {self.name}: {e}") finally: self.session = None + self._http_client = None class HttpStreamingServer(HttpBasedMcpServer): diff --git a/cecli/prompts/agent.yml b/cecli/prompts/agent.yml index ec1473e3f41..6cf5c35e844 100644 --- a/cecli/prompts/agent.yml +++ b/cecli/prompts/agent.yml @@ -64,7 +64,7 @@ main_system: | system_reminder: | - - Prefer the `ReadRange` tool to cli commands for file reading + - Prefer the `ReadFile` tool to cli commands for file reading - Pay close attention to indentation and styling when editing files - Batch tool calls as often as possible - Reason out loud through problems but be brief. diff --git a/cecli/prompts/subagent.yml b/cecli/prompts/subagent.yml index 6e3867a9852..74aa665bbf2 100644 --- a/cecli/prompts/subagent.yml +++ b/cecli/prompts/subagent.yml @@ -49,7 +49,7 @@ main_system: | system_reminder: | - - Prefer the `ReadRange` tool to cli commands for file reading + - Prefer the `ReadFile` tool to cli commands for file reading - Pay close attention to indentation and styling when editing files - Batch tool calls as often as possible - Reason out loud through problems but be brief. diff --git a/cecli/sendchat.py b/cecli/sendchat.py index e7ab59caf99..ad4ab56f919 100644 --- a/cecli/sendchat.py +++ b/cecli/sendchat.py @@ -196,7 +196,7 @@ def ensure_alternating_roles(messages): result.append(tool_msg) # Update prev_role to assistant after processing tool sequence - prev_role = "assistant" + prev_role = "tool" continue # Handle normal message alternation diff --git a/cecli/tools/__init__.py b/cecli/tools/__init__.py index 4791865349d..a9f8be77aa9 100644 --- a/cecli/tools/__init__.py +++ b/cecli/tools/__init__.py @@ -7,7 +7,7 @@ command, command_interactive, delegate, - edit_text, + edit_file, explore_code, git_branch, git_diff, @@ -17,7 +17,7 @@ git_status, grep, ls, - read_range, + read_file, resource_manager, thinking, undo_change, @@ -29,7 +29,7 @@ command, command_interactive, delegate, - edit_text, + edit_file, explore_code, _yield, git_branch, @@ -40,7 +40,7 @@ git_status, grep, ls, - read_range, + read_file, resource_manager, thinking, undo_change, diff --git a/cecli/tools/edit_text.py b/cecli/tools/edit_file.py similarity index 86% rename from cecli/tools/edit_text.py rename to cecli/tools/edit_file.py index f523241f606..2ce198a1382 100644 --- a/cecli/tools/edit_text.py +++ b/cecli/tools/edit_file.py @@ -2,6 +2,7 @@ ContentHashError, apply_hashline_operations, get_hashline_diff, + normalize_hashline, resolve_content_to_hashline_ids, strip_hashline, ) @@ -27,11 +28,12 @@ USER_EDIT_CATEGORIES = { "no_changes": "No Changes", "syntax_errors": "Syntax Errors", + "boundary_errors": "Boundary Resolution Error", } class Tool(BaseTool): - NORM_NAME = "edittext" + NORM_NAME = "editfile" TRACK_INVOCATIONS = False VALIDATIONS = { "edits": ["coerce_list"], @@ -40,7 +42,7 @@ class Tool(BaseTool): SCHEMA = { "type": "function", "function": { - "name": "EditText", + "name": "EditFile", "description": ( "Edit text in one or more files using content ID markers. " "You can perform multiple 'replace' or 'delete' operations in a single call. " @@ -75,6 +77,14 @@ class Tool(BaseTool): "or 'delete' to remove the ID range entirely." ), }, + "text": { + "type": "string", + "description": ( + "The exact replacement text. If operation is 'delete', " + 'this MUST be an empty string (""). ' + "NEVER include content IDs in this text." + ), + }, "start_line": { "type": "string", "description": ( @@ -89,21 +99,13 @@ class Tool(BaseTool): "(e.g., 'xyz::'). For empty files, use '@000'." ), }, - "text": { - "type": "string", - "description": ( - "The exact replacement text. If operation is 'delete', " - 'this MUST be an empty string (""). ' - "NEVER include content IDs in this text." - ), - }, }, "required": [ "file_path", "operation", + "text", "start_line", "end_line", - "text", ], }, }, @@ -138,19 +140,19 @@ def execute( message_dict=dict( role="user", content=( - "Please call `ReadRange` on files you intend to edit to" + "Please call `ReadFile` on files you intend to edit to" " make sure edits are appropriately targeted." ), ), tag=MessageTag.CUR, - hash_key=("edit_text", "reminder"), + hash_key=("edit_file", "reminder"), promotion=ConversationService.get_manager(coder).DEFAULT_TAG_PROMOTION_VALUE, mark_for_delete=0, mark_for_demotion=1, force=True, ) - tool_name = "EditText" + tool_name = "EditFile" try: # 1. Validate edits parameter if not isinstance(edits, list): @@ -198,29 +200,50 @@ def execute( "Must be 'replace' or 'delete'" ) - edit_text_raw = edit.get("text") - edit_text = edit.get("text") + edit_file_raw = edit.get("text") edit_start_line = edit.get("start_line") edit_end_line = edit.get("end_line") - if edit_text_raw is not None: - edit_text_raw = strip_hashline(edit_text_raw) - while edit_text_raw != edit_text: - edit_text_raw = strip_hashline(edit_text_raw) - edit_text = strip_hashline(edit_text) + # --------------------------------------------------------- + # DEFENSIVE FALLBACKS + # --------------------------------------------------------- + + # 1. Handle missing text parameter by defaulting to empty string + if edit_file_raw is None: + edit_file_raw = "" + + # 2. Programmatically enforce @000 for empty files + if not original_content or not original_content.strip(): + edit_start_line = "@000" + edit_end_line = "@000" - edit_text = edit_text_raw + # 3. Auto-sanitize malformed boundaries (strip accidentally appended code) + if isinstance(edit_start_line, str) and "::" in edit_start_line: + edit_start_line = normalize_hashline(edit_start_line) + if isinstance(edit_end_line, str) and "::" in edit_end_line: + edit_end_line = normalize_hashline(edit_end_line) + + # --------------------------------------------------------- + + edit_file = edit_file_raw + if edit_file_raw: + edit_file_raw = strip_hashline(edit_file_raw) + while edit_file_raw != edit_file: + edit_file_raw = strip_hashline(edit_file_raw) + edit_file = strip_hashline(edit_file) + + edit_file = edit_file_raw # Try to resolve line content values to content IDs - # This handles cases where LLMs pass actual line content - # instead of content ID markers edit_start_line, edit_end_line = resolve_content_to_hashline_ids( original_content, edit_start_line, edit_end_line ) # Validate required fields based on operation type + # (Note: The check for 'edit_file is None' will now be safely + # bypassed because we defaulted it to "" above) if operation in ("replace", "insert"): - if edit_text is None: + if edit_file is None: raise ToolError( f"Edit {edit_index + 1}: 'text' parameter is required for " f"'{operation}' operation" @@ -251,8 +274,8 @@ def execute( "end_line_hash": edit_end_line, "operation": operation, } - if edit_text is not None: - op_dict["text"] = edit_text + if edit_file is not None: + op_dict["text"] = edit_file operations.append(op_dict) @@ -261,7 +284,7 @@ def execute( "operation": operation, "start_line": edit_start_line, "end_line": edit_end_line, - "text": edit_text, + "text": edit_file, } file_metadata.append(metadata) @@ -285,16 +308,14 @@ def execute( else: # Be specific about why content didn't change if failed_ops: - error_details = "; ".join( - f"Edit {op['index'] + 1}: {op['error']}" for op in failed_ops - ) + error_details = "; ".join(op["error"] for op in failed_ops) raise ToolError( - f"Invalid Edit - Update content ID bounds: {error_details}" + f"Invalid Edit - Review content ID bounds: {error_details}" ) else: raise ToolError( - "Invalid Edit - Update content ID bounds - " - "all edits resulted in unchanged content" + "Invalid Edit - Review content ID bounds - " + "All edits resulted in unchanged content" ) if len(failed_ops): @@ -345,7 +366,7 @@ def execute( rel_path, original_content, new_content, - "edittext", + "editfile", metadata, change_id, ) @@ -494,6 +515,7 @@ def format_output(cls, coder, mcp_server, tool_response): end_line_hash=end_line, operation=operation, text=strip_hashline(text), + pretty=coder.agent_config.get("diff_colors", True), ) except ContentHashError: # diff_output = f"content ID verification failed: {str(e)}" @@ -515,18 +537,18 @@ def format_output(cls, coder, mcp_server, tool_response): @classmethod def _categorize_edit_error(cls, error_msg: str) -> str: - """Categorize an edit error message into a user-friendly display category. - - Maps errors from apply_hashline_operations to simplified category names - for user-facing output instead of displaying full error details. - - Args: - error_msg: The raw error message string. - - Returns: - str: The display category name (e.g., "No Changes", "Syntax Errors"). - """ + """Categorize an edit error message into a user-friendly display category.""" error_lower = error_msg.lower() + if "syntax error" in error_lower or "introduces new syntax" in error_lower: return USER_EDIT_CATEGORIES["syntax_errors"] - return USER_EDIT_CATEGORIES["no_changes"] + + elif "hash" in error_lower or "content id" in error_lower or "not found" in error_lower: + # Append the actual error string so the LLM can self-correct its specific mistake + return f"{USER_EDIT_CATEGORIES['boundary_errors']}: {error_msg}" + + elif "no changes" in error_lower: + return USER_EDIT_CATEGORIES["no_changes"] + + # Stop masking unknown errors; return them directly + return f"Edit Failed: {error_msg}" diff --git a/cecli/tools/grep.py b/cecli/tools/grep.py index c519cdd8d97..30d95426bb4 100644 --- a/cecli/tools/grep.py +++ b/cecli/tools/grep.py @@ -1,3 +1,6 @@ +import json +import os +import re import shutil from pathlib import Path @@ -10,6 +13,150 @@ from cecli.tools.utils.output import color_markers, tool_footer, tool_header from cecli.tools.validations import ToolValidations +# Default directories to exclude from search results across various languages +DEFAULT_EXCLUDE_DIRS = [ + ".git", + ".cecli", + ".venv", + "venv", + "env", + ".env", + "__pycache__", + "*.pyc", + "node_modules", + "bower_components", + ".next", + "dist", + "build", + "target", # Rust / Java / Kotlin + "bin", + "obj", # C# / .NET + ".gradle", # Java/Kotlin + ".mvn", + "vendor", # Go / PHP + ".bundle", + ".tox", + ".mypy_cache", + ".pytest_cache", + ".ruff_cache", + ".eggs", + "eggs", + "lib", + "lib64", + ".dub", # D + "dub.selections.json", + "Pods", # CocoaPods + ".build", # Swift + ".cargo", # Rust +] + + +def _build_exclude_args(tool_name, cmd_args): + """Add exclusion arguments for common build/artifact directories.""" + for exclude_dir in DEFAULT_EXCLUDE_DIRS: + if tool_name == "rg": + cmd_args.extend(["-g", f"!{exclude_dir}"]) + elif tool_name == "ag": + cmd_args.extend(["--ignore-dir", exclude_dir]) + elif tool_name == "grep": + cmd_args.extend(["--exclude-dir", exclude_dir]) + return cmd_args + + +def _parse_count_output(output): + """Parse grep -c output (file:count per line) into a dict.""" + counts = {} + if not output: + return counts + for line in output.splitlines(): + line = line.strip() + if not line: + continue + # Format: filepath:count + # Use rsplit to handle paths that may contain colons + idx = line.rfind(":") + if idx > 0: + filepath = line[:idx] + try: + count_val = int(line[idx + 1 :]) + counts[filepath] = count_val + except (ValueError, IndexError): + pass + return counts + + +def _parse_content_into_files(output): + """Parse grep -rn output into per-file groups. + + Returns list of dicts with keys: path, match_count, content_lines + Content lines preserve the original grep format (with : for matches, - for context). + """ + if not output: + return [] + + files = [] + lines = output.splitlines() + if not lines: + return [] + + current_file = None + current_lines = [] + match_count = 0 + + for i, line in enumerate(lines): + # Skip separator lines ("--" between non-contiguous match groups) + if line == "--": + continue + + # Try to extract filename from the line prefix + # Match lines: path:line:content + # Context lines: path-line-content (with - hyphen after line number) + m = re.match(r"^(.+?)[:-](\d+)[:-]", line) + if m: + filepath = m.group(1) + # Actually check: match lines have :LINE:, context lines have -LINE- + # The format is: path:line:content or path-line-content + # Check whether the char after line num is : or - + line_num_end = len(filepath) + 1 + len(m.group(2)) + is_match_line = line_num_end < len(line) and line[line_num_end] == ":" + + if current_file is None: + current_file = filepath + match_count = 1 if is_match_line else 0 + current_lines = [line] + elif filepath == current_file: + current_lines.append(line) + if is_match_line: + match_count += 1 + else: + # New file - save previous, start new + files.append( + { + "path": current_file, + "match_count": match_count, + "content": "\n".join(current_lines), + } + ) + current_file = filepath + match_count = 1 if is_match_line else 0 + current_lines = [line] + else: + # Line that doesn't match the pattern (e.g. rg --heading output) + if current_file is not None: + current_lines.append(line) + + # Don't forget the last file + if current_file is not None and current_lines: + files.append( + { + "path": current_file, + "match_count": match_count, + "content": "\n".join(current_lines), + } + ) + + return files + class Tool(BaseTool): NORM_NAME = "grep" @@ -46,7 +193,7 @@ class Tool(BaseTool): }, "use_regex": { "type": "boolean", - "default": True, + "default": False, "description": "Whether to use regex.", }, "case_insensitive": { @@ -54,6 +201,27 @@ class Tool(BaseTool): "default": True, "description": "Whether to perform a case-insensitive search.", }, + "count": { + "type": "boolean", + "default": True, + "description": ( + "Include match counts per file in the output summary." + ), + }, + "context_before": { + "type": "integer", + "default": 2, + "description": ( + "Number of context lines to show before each match." + ), + }, + "context_after": { + "type": "integer", + "default": 2, + "description": ( + "Number of context lines to show after each match." + ), + }, }, "required": ["pattern"], }, @@ -65,17 +233,67 @@ class Tool(BaseTool): }, } + @classmethod + def _validate_backend(cls, tool_name, tool_path): + """Test if a search backend actually works by running a quick check.""" + import subprocess + + try: + # Test with a simple pattern on a small known file + test_cmd = [tool_path, "--version"] + result = subprocess.run( + test_cmd, + capture_output=True, + timeout=5, + text=True, + ) + # Check that it returns successfully AND produces output + if result.returncode != 0: + return False + + # Also do a quick search test on a small file to detect hangs + grep_py = Path(__file__) + if grep_py.exists() and grep_py.stat().st_size < 100000: + search_test = [ + tool_path, + "-c", + "-F", + "import", + "--", + str(grep_py), + ] + if tool_name == "rg": + # rg -r is --replace, not recursive. rg is recursive by default. + # Use -c for count mode with separate flags + search_test = [tool_path, "--count", "--fixed-strings", "import", str(grep_py)] + elif tool_name == "ag": + search_test = [tool_path, "-c", "-Q", "import", str(grep_py)] + else: + search_test = [tool_path, "-c", "-r", "-F", "import", str(grep_py)] + + result2 = subprocess.run( + search_test, + capture_output=True, + timeout=5, + text=True, + ) + return result2.returncode in (0, 1) + + return True + except (subprocess.TimeoutExpired, OSError, Exception): + return False + @classmethod def _find_search_tool(self): """Find the best available command-line search tool (rg, ag, grep).""" - if shutil.which("rg"): - return "rg", shutil.which("rg") - elif shutil.which("ag"): - return "ag", shutil.which("ag") - elif shutil.which("grep"): - return "grep", shutil.which("grep") - else: - return None, None + candidates = ["rg", "ag", "grep"] + for name in candidates: + path = shutil.which(name) + if not path: + continue + if self._validate_backend(name, path): + return name, path + return None, None @classmethod def execute( @@ -87,152 +305,247 @@ def execute( """ Search for lines matching patterns in files within the project repository. Uses rg (ripgrep), ag (the silver searcher), or grep, whichever is available. + Returns a JSON string with structured results including per-file groupings, + match counts, and summary metadata. """ if not isinstance(searches, list): - # Handle legacy single-search call if necessary, or just error - return "Error: 'searches' parameter must be an array." + return json.dumps({"error": "'searches' parameter must be an array."}) repo = coder.repo if not repo: coder.io.tool_error("Not in a git repository.") - return "Error: Not in a git repository." + return json.dumps({"error": "Not in a git repository."}) tool_name, tool_path = cls._find_search_tool() if not tool_path: coder.io.tool_error("No search tool (rg, ag, grep) found in PATH.") - return "Error: No search tool (rg, ag, grep) found." + return json.dumps({"error": "No search tool (rg, ag, grep) found."}) + + all_operation_results = [] - all_results = [] for search_op in searches: pattern = strip_hashline(search_op.get("pattern")) file_pattern = search_op.get("file_glob", "*") directory = search_op.get("directory", search_op.get("path", ".")) - use_regex = search_op.get("use_regex", True) + use_regex = search_op.get("use_regex", False) case_insensitive = search_op.get("case_insensitive", True) context_before = search_op.get("context_before", 2) context_after = search_op.get("context_after", 2) + count_enabled = search_op.get("count", True) + + op_result = { + "pattern": pattern, + "file_glob": file_pattern, + "directory": directory, + "use_regex": use_regex, + "case_insensitive": case_insensitive, + "count": count_enabled, + "context_before": context_before, + "context_after": context_after, + "total_matches": 0, + "total_files": 0, + "has_more_files": False, + "error": None, + "files": [], + } try: search_dir_path = Path(repo.root) / directory - # Build the command arguments based on the available tool - cmd_args = [tool_path] - - # Common options or tool-specific equivalents - if tool_name in ["rg", "grep"]: - cmd_args.append("-n") # Line numbers for rg and grep - - if tool_name in ["rg"]: - cmd_args.append("--heading") # Filename above output for ripgrep - - # Context lines - if context_before > 0: - cmd_args.extend(["-B", str(context_before)]) - if context_after > 0: - cmd_args.extend(["-A", str(context_after)]) - - # Case sensitivity - if case_insensitive: - cmd_args.append("-i") + # Build base content command + base_cmd = [tool_path, "-n"] + if tool_name == "rg": + base_cmd.append("--with-filename") # Pattern type + pattern_flag = [] if use_regex: if tool_name == "grep": - cmd_args.append("-E") + pattern_flag = ["-E"] else: if tool_name == "rg": - cmd_args.append("-F") + pattern_flag = ["-F"] elif tool_name == "ag": - cmd_args.append("-Q") + pattern_flag = ["-Q"] elif tool_name == "grep": - cmd_args.append("-F") + pattern_flag = ["-F"] + + # Case sensitivity + case_flag = ["-i"] if case_insensitive else [] # File filtering + file_filter = [] if file_pattern != "*": if tool_name == "rg": - cmd_args.extend(["-g", file_pattern]) + file_filter = ["-g", file_pattern] elif tool_name == "ag": - cmd_args.extend(["-G", file_pattern]) + file_filter = ["-G", file_pattern] elif tool_name == "grep": - cmd_args.append("-r") - cmd_args.append(f"--include={file_pattern}") + file_filter = ["-r", f"--include={file_pattern}"] elif tool_name == "grep": - cmd_args.append("-r") - - if tool_name == "grep": - cmd_args.append("--exclude-dir=.git") - - # Add pattern and directory path - cmd_args.extend(["--", pattern, str(search_dir_path)]) - - command_string = oslex.join(cmd_args) + file_filter = ["-r"] + + # Exclusions + exclude_args = [] + _build_exclude_args(tool_name, exclude_args) + + # --- PASS 1: Get match counts (fast, no context) --- + counts = {} + if count_enabled: + # Build count command (separate flags to avoid -r confusion) + # NOTE: rg -r = --replace (takes arg), not recursive. rg is recursive by default. + count_cmd_parts = [tool_path] + if tool_name == "rg": + count_cmd_parts.append("-c") + elif tool_name == "ag": + count_cmd_parts.append("-rc") + else: + count_cmd_parts.append("-rnc") + count_cmd = ( + count_cmd_parts + + case_flag + + pattern_flag + + exclude_args + + file_filter + + ["--", pattern, str(search_dir_path)] + ) + count_string = oslex.join(count_cmd) + coder.io.tool_output( + f"⛭ Counting matches with {tool_name}: '{pattern}' in {directory}", + type="tool-result", + ) + count_status, count_output = run_cmd_subprocess( + count_string, + verbose=coder.verbose, + cwd=coder.root, + should_print=False, + ) + if count_status == 0: + counts = _parse_count_output(count_output) + + # --- PASS 2: Get content with context --- + content_cmd = ( + base_cmd + + (["-B", str(context_before)] if context_before > 0 else []) + + (["-A", str(context_after)] if context_after > 0 else []) + + case_flag + + pattern_flag + + exclude_args + + file_filter + + ["--", pattern, str(search_dir_path)] + ) + content_string = oslex.join(content_cmd) coder.io.tool_output( - f"⛭ Executing {tool_name}: {command_string}", type="tool-result" + f"⛭ Executing {tool_name}: '{pattern}' in {directory}", + type="tool-result", ) - - exit_status, combined_output = run_cmd_subprocess( - command_string, + content_status, content_output = run_cmd_subprocess( + content_string, verbose=coder.verbose, cwd=coder.root, should_print=False, ) - output_content = combined_output or "" - - if exit_status == 0: - max_output_lines = 50 - output_lines = output_content.splitlines() - if len(output_lines) > max_output_lines: - truncated_output = "\n".join(output_lines[:max_output_lines]) - all_results.append( - f"Matches for '{pattern}'" - f" (truncated):\n```text\n{truncated_output}\n..." - f" ({len(output_lines) - max_output_lines} more lines)\n```" - ) + output_content = content_output or "" + + if content_status == 0 and output_content: + parsed_files = _parse_content_into_files(output_content) + + # Merge in counts from pass 1 if available + if counts: + for pf in parsed_files: + raw_path = pf["path"] + if raw_path in counts: + pf["count_from_pass"] = counts[raw_path] + else: + # Try with repo root prefix stripped + rel = os.path.relpath(raw_path, repo.root) + pf["count_from_pass"] = counts.get(rel, pf["match_count"]) else: - all_results.append( - f"Matches for '{pattern}':\n```text\n{output_content}\n```" - ) - elif exit_status == 1: - all_results.append(f"No matches found for '{pattern}'.") + for pf in parsed_files: + pf["count_from_pass"] = pf["match_count"] + + # Apply file-based truncation: + MAX_MATCHES_PER_FILE = 10 + MAX_FILES = 20 + + truncated_files = [] + total_matches = 0 + total_files = 0 + + for pf in parsed_files[:MAX_FILES]: + file_lines = pf["content"].splitlines() + # Find actual match lines (lines with `:LINE:` pattern) + filepath_escaped = re.escape(pf["path"]) + match_line_re = re.compile(r"^" + filepath_escaped + r":\d+:") + match_lines_found = [ln for ln in file_lines if match_line_re.match(ln)] + + if len(match_lines_found) > MAX_MATCHES_PER_FILE: + truncated_files.append(pf["path"]) + trimmed = "\n".join(match_lines_found[:MAX_MATCHES_PER_FILE]) + pf["content"] = trimmed + + # Normalize path to be relative to repo root + pf["path"] = os.path.relpath(pf["path"], repo.root) + total_matches += pf.get("count_from_pass", 0) + total_files += 1 + + has_more = len(parsed_files) > MAX_FILES + if has_more: + for pf in parsed_files[MAX_FILES:]: + total_matches += pf.get("count_from_pass", 0) + total_files += 1 + + op_result["total_matches"] = total_matches + op_result["total_files"] = total_files + op_result["has_more_files"] = has_more + op_result["files"] = [ + { + "path": pf["path"], + "match_count": pf.get("count_from_pass", 0), + "truncated": pf["path"] in truncated_files, + "content": pf["content"], + } + for pf in parsed_files[:MAX_FILES] + ] + + elif content_status == 1 or not output_content: + op_result["total_matches"] = 0 + op_result["total_files"] = 0 else: - all_results.append(f"Error searching for '{pattern}': {output_content}") + op_result["error"] = output_content except Exception as e: - all_results.append(f"Error executing search for '{pattern}': {str(e)}") + op_result["error"] = f"Error executing search: {str(e)}" + + all_operation_results.append(op_result) - final_message = "\n\n".join(all_results) + final_result = {"operations": all_operation_results} + # TUI summary if coder.tui and coder.tui(): - # For the UI, show a summary to avoid cluttering the terminal ui_summaries = [] - for search_op, result in zip(searches, all_results): - pattern = search_op.get("pattern") - if "No matches found" in result: + for op in all_operation_results: + pattern = op["pattern"] + if op["error"]: + ui_summaries.append(f"✗ Error searching for '{pattern}': {op['error']}") + elif op["total_matches"] == 0: ui_summaries.append(f"✗ No matches found for '{pattern}'.") - elif "Error" in result: - ui_summaries.append(f"✗ Error searching for '{pattern}'.") else: - # Count lines in the output to give a sense of scale - # The result string contains the matches in a code block - match_count = ( - result.count("\n") - 2 - ) # Subtracting for the markdown block markers - if match_count < 0: - match_count = 0 - ui_summaries.append(f"✓ Matches found for '{pattern}'.") - + ui_summaries.append( + f"✓ '{pattern}': {op['total_matches']} matches in " + f"{op['total_files']} files" + ) ui_message = "\n".join(ui_summaries) coder.io.tool_output(ui_message, type="tool-result") - return final_message + return json.dumps(final_result) @classmethod def format_output(cls, coder, mcp_server, tool_response): - """Format output for Grep tool.""" + """Format the search parameters for TUI display.""" color_start, color_end = color_markers(coder) - # Output header tool_header(coder=coder, mcp_server=mcp_server, tool_response=tool_response) try: @@ -243,7 +556,7 @@ def format_output(cls, coder, mcp_server, tool_response): coder.io.tool_error("Invalid Tool JSON") return - # Output each search operation with the requested format + # Display each search operation searches = params.get("searches", []) if searches: coder.io.tool_output("") @@ -252,29 +565,25 @@ def format_output(cls, coder, mcp_server, tool_response): file_pattern = search_op.get("file_glob", "*") directory = search_op.get("directory", ".") use_regex = search_op.get("use_regex", False) - case_insensitive = search_op.get("case_insensitive", False) - context_before = search_op.get("context_before", 5) - context_after = search_op.get("context_after", 5) + case_insensitive = search_op.get("case_insensitive", True) + context_before = search_op.get("context_before", 2) + context_after = search_op.get("context_after", 2) - # Format as "search: • pattern • file_pattern • directory • options" formatted_query = ( f"{color_start}search_{i + 1}:{color_end} {pattern} • {file_pattern} •" f" {directory}" ) - - # Add options if they differ from defaults options = [] if use_regex: options.append("regex") if case_insensitive: options.append("case-insensitive") - if context_before != 5 or context_after != 5: + if context_before != 2 or context_after != 2: options.append(f"context:{context_before}/{context_after}") - if options: formatted_query += f" • {' '.join(options)}" - coder.io.tool_output(formatted_query) + coder.io.tool_output("") tool_footer(coder=coder, tool_response=tool_response, params=params) diff --git a/cecli/tools/ls.py b/cecli/tools/ls.py index e5e12c9915b..4305c5aea90 100644 --- a/cecli/tools/ls.py +++ b/cecli/tools/ls.py @@ -65,7 +65,7 @@ def execute(cls, coder, path=None, **kwargs): try: with os.scandir(abs_path) as entries: for entry in entries: - if entry.is_file() and not entry.name.startswith("."): + if not entry.name.startswith("."): rel_path = os.path.relpath(entry.path, coder.root) contents.append(rel_path) except OSError as e: @@ -80,9 +80,11 @@ def execute(cls, coder, path=None, **kwargs): f"🗐 Listed {len(contents)} file(s) in '{dir_path}'", type="tool-result" ) sorted_contents = sorted(contents) - if len(sorted_contents) > 10: + if len(sorted_contents) > 500: return ( - f"Found {len(sorted_contents)} files: {', '.join(sorted_contents[:10])}..." + f"Found {len(sorted_contents)} files:" + f" {', '.join(sorted_contents[:500])}" + f"\n... and {len(sorted_contents) - 500} more" ) else: return f"Found {len(sorted_contents)} files: {', '.join(sorted_contents)}" diff --git a/cecli/tools/read_range.py b/cecli/tools/read_file.py similarity index 98% rename from cecli/tools/read_range.py rename to cecli/tools/read_file.py index 50cc56abdb0..0ffc04ca2fc 100644 --- a/cecli/tools/read_range.py +++ b/cecli/tools/read_file.py @@ -15,7 +15,7 @@ class Tool(BaseTool): - NORM_NAME = "readrange" + NORM_NAME = "readfile" TRACK_INVOCATIONS = False VALIDATIONS = { "read": ["coerce_list"], @@ -26,10 +26,9 @@ class Tool(BaseTool): SCHEMA = { "type": "function", "function": { - "name": "ReadRange", + "name": "ReadFile", "description": ( "Get content ID prefixed content between start and end markers in files." - " This is useful for files you are attempting to edit and for understanding their structure." " Accepts an array of `read` objects, each with file_path, range_start, range_end." " They can contain up to 3 lines of content. Avoid using singular generic keywords and" " symbols. Special markers @000 and 000@ represent the file boundaries and can be" @@ -37,10 +36,12 @@ class Tool(BaseTool): " respectively. Line numbers may also be used for range lookups." " It is best to use function names, variable declarations, entire line contents" " and other meaningful identifiers as range_start and range_end values." + " Results may be modified from the raw search for semantic relevance." + " The returned identifiers will not persist after editing the file." " Do not use the same pattern for the range_start and range_end." " Do not use empty strings for the range_start and range_end." " Do not use content IDs for the range_start and range_end values as they change between edits." - " Always use the ReadRange tool instead of cli tools for reading file contents." + " Always use the ReadFile tool instead of cli tools for reading file contents." " Line number and special marker ranges greater than 200 lines will return" " preview content for further, more scoped investigation." " Call this tool sequentially on increasingly finer grained searches " @@ -97,7 +98,7 @@ def execute(cls, coder, read, **kwargs): """ from cecli.helpers.conversation import ConversationService - tool_name = "ReadRange" + tool_name = "ReadFile" already_up_to_date = [] new_context_retrieved = [] error_outputs = [] @@ -212,9 +213,9 @@ def execute(cls, coder, read, **kwargs): [ f"File {rel_path} is empty.", ( - "Next: use EditText with start_line @000 and end_line @000 to" + "Next: use EditFile with start_line @000 and end_line @000 to" " write content, or ResourceManager to scaffold — do not call" - " ReadRange again on this empty file." + " ReadFile again on this empty file." ), ] ) @@ -649,7 +650,7 @@ def _is_valid_int(s): ) if already_up_to_date and not new_context_retrieved: result_parts.append( - "Do not call `ReadRange` again with these parameters again unless you edit" + "Do not call `ReadFile` again with these parameters again unless you edit" " the relevant files." ) @@ -940,7 +941,7 @@ def clear_old_messages(cls, coder): @classmethod def format_output(cls, coder, mcp_server, tool_response): - """Format output for ReadRange tool.""" + """Format output for ReadFile tool.""" color_start, color_end = color_markers(coder) # Output header @@ -974,7 +975,7 @@ def format_output(cls, coder, mcp_server, tool_response): @classmethod def format_error(cls, coder, error_text, file_path, range_start, range_end, operation_index): - """Format error output for the ReadRange tool.""" + """Format error output for the ReadFile tool.""" # Truncate range_start to first line with ellipsis if multiline start_line = (range_start or "N/A").split("\n")[0] diff --git a/cecli/tools/utils/helpers.py b/cecli/tools/utils/helpers.py index 71ed132beb3..559adec9a15 100644 --- a/cecli/tools/utils/helpers.py +++ b/cecli/tools/utils/helpers.py @@ -338,8 +338,8 @@ def format_tool_result( result_for_llm += f" Change ID: {change_id}." if diff_snippet: result_for_llm += f" Diff snippet:\n{diff_snippet}" - else: - result_for_llm += " A diff will be provided in a future message." + # else: + # result_for_llm += " A diff will be provided in a future message." return result_for_llm diff --git a/cecli/tools/utils/registry.py b/cecli/tools/utils/registry.py index f582a617f75..2ef5b6987f1 100644 --- a/cecli/tools/utils/registry.py +++ b/cecli/tools/utils/registry.py @@ -19,7 +19,7 @@ class ToolRegistry: """Registry for tool discovery and management.""" _tools: Dict[str, Type] = {} # normalized name -> Tool class - _essential_tools: Set[str] = {"resourcemanager", "edittext", "yield"} + _essential_tools: Set[str] = {"resourcemanager", "editfile", "yield"} _registry: Dict[str, Type] = {} # cached filtered registry loaded_custom_tools: List[str] = [] diff --git a/cecli/tui/__init__.py b/cecli/tui/__init__.py index d0723373302..58773b1dec1 100644 --- a/cecli/tui/__init__.py +++ b/cecli/tui/__init__.py @@ -56,6 +56,7 @@ def create_tui_io(args, editing_mode): multiline_mode=args.multiline, notifications=args.notifications, notifications_command=args.notifications_command, + notification_bell=args.notification_bell, verbose=args.verbose, ) diff --git a/cecli/website/docs/config.md b/cecli/website/docs/config.md index 1412b27f815..4efb5e8f7f3 100644 --- a/cecli/website/docs/config.md +++ b/cecli/website/docs/config.md @@ -41,34 +41,38 @@ CECLI_DARK_MODE=true ``` -## Retries +## Default `~/.cecli/` Locations -cecli can be configured to retry failed API calls. -This is useful for handling intermittent network issues or other transient errors. -The `retries` option is a JSON object that can be configured with the following keys: +cecli also checks several default locations inside `~/.cecli/` for +configuration, environment variables, and agent resources. +These are always included with lower precedence than project-level equivalents, +so a setting in a project `.cecli.conf.yml` or `.env` file will override the +`~/.cecli/` default. -- `retry-timeout`: The timeout in seconds for each retry. -- `retry-backoff-factor`: The backoff factor to use between retries. -- `retry-on-unavailable`: Whether to retry on 503 Service Unavailable errors. +### `~/.cecli/conf.yml` -Example usage in `.cecli.conf.yml`: +A YAML configuration file read after all other config file sources, +making it the lowest-precedence config option. Useful for setting +machine-wide defaults that individual projects can override. +See the [configuration section](/docs/config/conf.html) above for supported options. -```yaml -retries: - retry-timeout: 30 - retry-backoff-factor: 1.50 - retry-on-unavailable: true -``` +### `~/.cecli/.env` -This can also be set with the `--retries` command line switch, passing a JSON string: +An environment file loaded before any other `.env` file, so project-level +`.env` files can override its values. A convenient place to store +API keys or other environment variables used across multiple projects. -``` -$ cecli --retries '{"retry-timeout": 30, "retry-backoff-factor": 1.50, "retry-on-unavailable": true}' -``` +### `~/.cecli/skills/` + +A directory containing skill packages (each a sub-directory with a +`SKILL.md` file). Skills here are discoverable alongside those in any +user-configured skills paths. + +### `~/.cecli/subagents/` + +A directory containing sub-agent definition files (`.md` files with +YAML front matter). Sub-agents here are registered alongside those in +any user-configured sub-agent paths. -Or by setting the `CECLI_RETRIES` environment variable: -``` -export CECLI_RETRIES='{"retry-timeout": 30, "retry-backoff-factor": 1.50, "retry-on-unavailable": true}' -``` {% include keys.md %} diff --git a/cecli/website/docs/config/agent-mode.md b/cecli/website/docs/config/agent-mode.md index fffe26c748d..2d571ee697f 100644 --- a/cecli/website/docs/config/agent-mode.md +++ b/cecli/website/docs/config/agent-mode.md @@ -47,7 +47,7 @@ This loop continues automatically until the `Yield` tool is called, or the maxim Agent Mode uses a centralized local tool registry that manages all available tools: - **File Discovery Tools**: `ExploreCode`, `Ls`, `Grep` -- **Editing Tools**: `EditText`, +- **Editing Tools**: `EditFile`, - **Context Management Tools**: `ResourceManager`, `GetLines` - **Git Tools**: `GitDiff`, `GitLog`, `GitShow`, `GitStatus` - **Utility Tools**: `UpdateTodoList`, `UndoChange`, `Yield` @@ -113,10 +113,10 @@ Files are made editable and modifications are applied: Tool Call: MakeEditable Arguments: {"file_path": "main.py"} -Tool Call: EditText +Tool Call: EditFile Arguments: {"file_path": "main.py", "find_text": "old_function", "replace_text": "new_function"} -Tool Call: EditText +Tool Call: EditFile Arguments: {"file_path": "main.py", "after_pattern": "import statements", "content": "new_imports"} ``` @@ -164,13 +164,14 @@ Agent Mode can also be configured directly in your configuration file. See the [ - **`exclude_context_blocks`**: Array of context block names to exclude from default set - **`hot_reload`**: When enabled, skills configuration is hot-reloaded automatically, reflecting changes to skills without requiring a restart (default: false) - **`command_timeout`**: Time in seconds to wait for shell commands to finish before automatic backgrounding occurs (default: None) +- **`diff_colors`**: When enabled, diff output in edit tool responses uses color-coded lines - removed lines in magenta, added lines in light green, and context lines in plain text (default: true) #### Essential Tools Certain tools are always available regardless of includelist/excludelist settings: - `ResourceManager` - Add, drop, and make files editable in the context -- `edittext` - Basic text replacement +- `editfile` - Basic text replacement - `finished` - Complete the task The registry also supports **Custom Tools** that can be loaded from specified directories or files using the `tool_paths` configuration option. Custom tools must be Python files containing a `Tool` class that inherits from `BaseTool` and defines a `NORM_NAME` attribute. @@ -264,7 +265,7 @@ agent: true # Agent Mode configuration agent-config: # Tool configuration - tools_includelist: ["resourcemanager", "edittext", "finished"] # Optional: Whitelist of tools + tools_includelist: ["resourcemanager", "editfile", "finished"] # Optional: Whitelist of tools tools_excludelist: ["command", "commandinteractive"] # Optional: Blacklist of tools tools_paths: ["./custom-tools", "~/my-tools"] # Optional: Directories or files containing custom tools @@ -287,6 +288,7 @@ agent-config: allowed_commands: ["wc -l*"] # Commands matching these glob patterns will not prompt for confirmation show_lint_errors: false # When enabled, linting errors are shown in tool output (default: false) hot_reload: false # When enabled, skills configuration is hot-reloaded automatically (default: false) +фвδ:: diff_colors: true # When enabled, diff output uses color-coded lines (default: true) # Skills configuration (see Skills documentation for details) skills_paths: ["~/my-skills", "./project-skills"] # Directories to search for skills skills_includelist: ["python-refactoring", "react-components"] # Optional: Whitelist of skills to include diff --git a/cecli/website/docs/config/hooks.md b/cecli/website/docs/config/hooks.md index 636a6e8a34f..1ac4a32e931 100644 --- a/cecli/website/docs/config/hooks.md +++ b/cecli/website/docs/config/hooks.md @@ -91,19 +91,22 @@ Python hooks allow for more complex logic. To create a Python hook, create a `.p **File**: `.cecli/hooks/my_hook.py` ```python -from cecli.hooks import BaseHook +from cecli.hooks import BaseHook, HookHelpers from cecli.hooks.types import HookType class MyCustomHook(BaseHook): type = HookType.PRE_TOOL - + async def execute(self, coder, metadata): # Access coder instance or metadata tool_name = metadata.get("tool_name") - + if tool_name == "delete_file": print("Warning: Deleting a file!") - + + # Fetch recent messages for context + recent = HookHelpers.get_messages(coder, last_n=3) + # Return False to abort operation (for pre_tool/post_tool) return True ``` @@ -116,6 +119,88 @@ hooks: file: .cecli/hooks/my_hook.py ``` +## Hook Helpers + +The ``HookHelpers`` class provides a higher-level API for writing Python hooks. +All helpers are accessed through a single import — ``from cecli.hooks import HookHelpers`` — +giving you convenient access to conversation history, model calls, and sub-agent +invocation from within any hook's ``execute()`` method. + +```python +from cecli.hooks import BaseHook, HookHelpers +from cecli.hooks.types import HookType + +class MyHook(BaseHook): + type = HookType.POST_TOOL + + async def execute(self, coder, metadata): + # Fetch recent conversation messages + recent = HookHelpers.get_messages(coder, last_n=5) + + # Make an LLM call + reply = await HookHelpers.call(coder, prompt="Summarize the last message.") + + # Append a message to the conversation + HookHelpers.append_message(coder, {"role": "user", "content": reply}) + + # Invoke a sub-agent + summary = await HookHelpers.call_subagent( + coder, "reviewer", "Review these changes" + ) + return True +``` + +### get_messages(coder, last_n=None, tag=None, reload=False) + +Retrieve messages from the agent's conversation history as a list of message +dicts (``{"role": …, "content": …}``). + +| Parameter | Description | +|-----------|-------------| +| ``coder`` | The coder instance passed to ``execute()``. | +| ``last_n`` | If set, return only the *last_n* messages (most recent). | +| ``tag`` | Optional tag to filter by (e.g. ``"cur"``, ``"done"``). | +| ``reload`` | If ``True``, bypass the internal cache. | + +### append_message(coder, message_dict, tag="cur", **kwargs) + +Append a message to the agent's conversation history. The message will be +visible to the LLM on the next turn. Returns the ``BaseMessage`` instance. + +| Parameter | Description | +|-----------|-------------| +| ``coder`` | The coder instance passed to ``execute()``. | +| ``message_dict`` | The message content dict, e.g. ``{"role": "user", "content": "..."}``. | +| ``tag`` | Message tag (default ``"cur"``). | +| ``**kwargs`` | Extra arguments like ``hash_key``, ``force``, ``priority``. | + +### call(coder, messages=None, prompt=None, system=None, model_name=None, max_tokens=None, **kwargs) + +Make a language model generation call (async). You can either pass a pre-built +``messages`` list, or use ``prompt`` with an optional ``system`` preamble. + +| Parameter | Description | +|-----------|-------------| +| ``coder`` | The coder instance passed to ``execute()``. | +| ``messages`` | Full message list (overrides ``prompt``/``system``). | +| ``prompt`` | A simple user-prompt string. | +| ``system`` | Optional system message prepended to ``prompt``. | +| ``model_name`` | Override model (e.g. ``"gpt-4o"``). Defaults to ``coder.main_model``. | +| ``max_tokens`` | Maximum response tokens. | +| ``**kwargs`` | Extra arguments passed to the underlying ``simple_send_with_retries()``. | + +### call_subagent(coder, name, prompt, **kwargs) + +Invoke a registered sub-agent by name (async, blocking by default). Returns +the sub-agent's summary string, or ``None`` on failure. + +| Parameter | Description | +|-----------|-------------| +| ``coder`` | The coder instance passed to ``execute()``. | +| ``name`` | The registered sub-agent name (e.g. ``"reviewer"``, ``"tester"``). | +| ``prompt`` | The user message to send to the sub-agent. | +| ``**kwargs`` | Extra arguments like ``blocking``, ``parent``, ``auto_reap``. | + ## Managing Hooks You can manage hooks during an active session using the following slash commands: diff --git a/cecli/website/docs/config/mcp.md b/cecli/website/docs/config/mcp.md index 71d7e49b181..550dd7c69d0 100644 --- a/cecli/website/docs/config/mcp.md +++ b/cecli/website/docs/config/mcp.md @@ -14,6 +14,24 @@ cecli supports configuring MCP servers using the MCP Server Configuration schema see the [Model Context Protocol documentation](https://modelcontextprotocol.io/introduction) for more information. +### Keepalive Mechanism + +For HTTP-based servers, you can enable a keepalive mechanism to prevent connections from dropping during long idle periods. This is done by adding the `keepalive_interval` property to your server configuration. + +- `keepalive_interval`: (Optional) An integer specifying the interval in seconds for sending a heartbeat (an `OPTIONS` request) to the server. + - If not provided, it defaults to **300** seconds. + - The value must be between **5** and **300** seconds. + +Example with keepalive enabled: +```yaml +mcp-servers: + mcpServers: + context7: + transport: http + url: https://mcp.context7.com/mcp + keepalive_interval: 60 # Send a heartbeat every 60 seconds +``` + You have two ways of sharing your MCP server configuration with cecli. {: .note } diff --git a/cecli/website/docs/config/model-providers.md b/cecli/website/docs/config/model-providers.md new file mode 100644 index 00000000000..9fd85477d0f --- /dev/null +++ b/cecli/website/docs/config/model-providers.md @@ -0,0 +1,180 @@ +--- +parent: Configuration +nav_order: 800 +description: Configure custom OpenAI-compatible model providers. +--- + +# Model Providers + +Model providers allow you to use OpenAI-compatible API endpoints with cecli. You can define custom providers that point to any OpenAI-compatible API, including self-hosted models, proprietary endpoints, or third-party services. + +Built-in providers are defined in [`providers.json`](https://github.com/cecli/cecli/blob/main/cecli/resources/providers.json) and are automatically available. Use the `--model-providers` option to add your own. + +## Configuration Format + +Each provider is defined as a JSON object with the following keys: + +| Key | Required | Description | +|-----|----------|-------------| +| `api_base` | Yes | The base URL for the OpenAI-compatible API endpoint | +| `api_key_env` | Yes | A list of environment variable names that can hold the API key (the first found one is used) | +| `display_name` | No | A human-readable name for the provider | +| `models_url` | No | URL to fetch the model list (defaults to `{api_base}/models`) | +| `default_headers` | No | A dict of default HTTP headers to include in every request | +| `account_id_env` | No | Environment variable name for an account ID (used with `{account_id}` in `models_url` or `api_base`) | +| `static_models` | No | A list of static model definitions when no `models_url` is available | +| `hf_namespace` | No | If `true`, model names are treated as HuggingFace repository identifiers (prefixed with `hf:`) | +| `supports_stream` | No | Set to `false` if the provider does not support streaming responses | +| `requires_api_key` | No | Set to `false` if the provider does not require an API key (default: `true`) | +| `base_url_env` | No | A list of environment variable names that can override `api_base` (takes precedence if set) | + + +## Configuration File Usage + +You can also define model providers in your `~/.cecli/conf.yml` or `.cecli.conf.yml` configuration file: + +```yaml +model-providers: + my-provider: + api_base: "https://api.myprovider.com/v1" + api_key_env: + - "MY_PROVIDER_API_KEY" + display_name: "My Provider" +``` + +## Command Line Usage + +Use the `--model-providers` option to define custom providers as a JSON string: + +```bash +cecli --model-providers '{ + "my-provider": { + "api_base": "https://api.myprovider.com/v1", + "api_key_env": ["MY_PROVIDER_API_KEY"], + "display_name": "My Provider" + } +}' +``` + +## Using Custom Provider Models + +Once a provider is configured, you can use its models by referencing them with the provider slug as a prefix: + +```bash +cecli --model my-provider/model-name +``` + +For example, if you configure provider `my-provider` and it serves a model called `gpt-4o-mini`, you would use: + +```bash +cecli --model my-provider/gpt-4o-mini +``` + +## Setting API Keys + +Set the environment variable(s) specified in `api_key_env` before running cecli: + +```bash +export MY_PROVIDER_API_KEY="sk-your-key-here" +cecli --model my-provider/gpt-4o-mini +``` + + +## Advanced Configuration + +### Custom Model List Endpoint + +If your provider uses a different endpoint for listing models, specify `models_url`: + +```yaml +model-providers: + my-provider: + api_base: "https://api.myprovider.com/v1" + api_key_env: ["MY_PROVIDER_API_KEY"] + models_url: "https://api.myprovider.com/v1/models/list" +``` + +### Static Model Definitions + +If your provider does not expose a `/models` endpoint, define models statically: + +```yaml +model-providers: + my-provider: + api_base: "https://api.myprovider.com/v1" + api_key_env: ["MY_PROVIDER_API_KEY"] + static_models: + - id: "gpt-4o" + max_input_tokens: 128000 + mode: "chat" + - id: "gpt-4o-mini" + max_input_tokens: 128000 + mode: "chat" +``` + +### Default Headers + +Add default HTTP headers for every request: + +```yaml +model-providers: + my-provider: + api_base: "https://api.myprovider.com/v1" + api_key_env: ["MY_PROVIDER_API_KEY"] + default_headers: + X-Custom-Header: "value" + X-Organization: "my-org" +``` + +### HuggingFace Namespace + +For providers that serve HuggingFace models, enable the `hf_namespace` flag: + +```yaml +model-providers: + my-hf-provider: + api_base: "https://api.myprovider.com/v1" + api_key_env: ["MY_PROVIDER_API_KEY"] + hf_namespace: true +``` + +This will prefix model names with `hf:` (e.g., `hf:meta-llama/Llama-2-7b`). + +## Built-in Providers + +cecli ships with several built-in providers defined in `providers.json`. These are automatically available without any configuration: + +| Provider | Slug | API Base | +|----------|------|----------| +| Apertis | `apertis` | `https://api.stima.tech/v1` | +| Chutes | `chutes` | `https://llm.chutes.ai/v1/` | +| Fireworks AI | `fireworks_ai` | `https://api.fireworks.ai/inference/v1` | +| Helicone | `helicone` | `https://ai-gateway.helicone.ai/` | +| Nano-GPT | `nano-gpt` | `https://nano-gpt.com/api/v1` | +| Poe | `poe` | `https://api.poe.com/v1` | +| PublicAI | `publicai` | `https://api.publicai.co/v1` | +| Synthetic | `synthetic` | `https://api.synthetic.new/openai/v1` | +| Venice AI | `veniceai` | `https://api.venice.ai/api/v1` | +| Xiaomi MiMo | `xiaomi_mimo` | `https://api.xiaomimimo.com/v1` | + +## Troubleshooting + +### "Provider not found" + +Ensure the provider slug is correctly spelled and that you are using it as a prefix in the model name: + +```bash +cecli --model my-provider/model-name +``` + +### "Missing API key" + +Set the appropriate environment variable defined in `api_key_env` before running cecli: + +```bash +export MY_PROVIDER_API_KEY="sk-..." +``` + +### "Connection refused" or timeout errors + +Verify that the `api_base` URL is correct and accessible from your network. Some providers may require VPN access or specific network configuration. \ No newline at end of file diff --git a/cecli/website/docs/config/retries.md b/cecli/website/docs/config/retries.md new file mode 100644 index 00000000000..606bd31c660 --- /dev/null +++ b/cecli/website/docs/config/retries.md @@ -0,0 +1,37 @@ +--- +parent: Configuration +nav_order: 600 +description: How to configure cecli retry behavior for failed API calls. +--- + +# Retries + +cecli can be configured to retry failed API calls. +This is useful for handling intermittent network issues or other transient errors. +The `retries` option is a JSON object that can be configured with the following keys: + +- `retry-timeout`: The timeout in seconds for each retry. +- `retry-backoff-factor`: The backoff factor to use between retries. +- `retry-on-unavailable`: Whether to retry on 503 Service Unavailable errors. + +Example usage in `.cecli.conf.yml`: + +```yaml +retries: + retry-timeout: 30 + retry-backoff-factor: 1.50 + retry-on-unavailable: true +``` + +This can also be set with the `--retries` command line switch, passing a JSON string: + +``` +$ cecli --retries '{"retry-timeout": 30, "retry-backoff-factor": 1.50, "retry-on-unavailable": true}' +``` + +Or by setting the `CECLI_RETRIES` environment variable: + +``` +export CECLI_RETRIES='{"retry-timeout": 30, "retry-backoff-factor": 1.50, "retry-on-unavailable": true}' +``` +{% include keys.md %} \ No newline at end of file diff --git a/tests/basic/test_skills.py b/tests/basic/test_skills.py index 8f6e5f52b62..b7e24f5d082 100644 --- a/tests/basic/test_skills.py +++ b/tests/basic/test_skills.py @@ -31,7 +31,8 @@ def test_skills_manager_initialization(self): """Test that SkillsManager initializes correctly.""" # Test with empty directory paths manager = SkillsManager([]) - assert manager.directory_paths == [] + + assert manager.directory_paths == [Path.home() / ".cecli" / "skills"] assert manager.include_list is None assert manager.exclude_list == set() assert manager.git_root is None @@ -40,7 +41,8 @@ def test_skills_manager_initialization(self): # Test with directory paths manager = SkillsManager(["/tmp/test"]) - assert len(manager.directory_paths) == 1 + # "/tmp/test" + default home dir = 2 paths + assert len(manager.directory_paths) == 2 assert isinstance(manager.directory_paths[0], Path) assert manager._loaded_skills == set() @@ -51,6 +53,8 @@ def test_skills_manager_initialization(self): exclude_list=["skill3"], git_root="/tmp", ) + # "/tmp/test" + default home dir = 2 paths + assert len(manager.directory_paths) == 2 assert manager.include_list == {"skill1", "skill2"} assert manager.exclude_list == {"skill3"} assert manager.git_root == Path("/tmp").expanduser().resolve() diff --git a/tests/coders/test_coder_switching.py b/tests/coders/test_coder_switching.py index efefcd8ef50..603be5f4597 100644 --- a/tests/coders/test_coder_switching.py +++ b/tests/coders/test_coder_switching.py @@ -31,7 +31,7 @@ async def run_test(): main_model.reasoning_tag = "think" main_model.get_active_model.return_value = main_model - mock_tool_registry.get_registered_tools.return_value = ["edittext"] + mock_tool_registry.get_registered_tools.return_value = ["editfile"] mock_tool_registry.get_tool.return_value = MagicMock() mock_tool_registry.build_registry.return_value = None diff --git a/tests/mcp/conftest.py b/tests/mcp/conftest.py new file mode 100644 index 00000000000..dfb5b95623c --- /dev/null +++ b/tests/mcp/conftest.py @@ -0,0 +1,138 @@ +from typing import Any, AsyncGenerator, Dict +from unittest.mock import MagicMock + +import pytest + +# Allow short keepalive intervals in tests +import cecli.mcp.server as mcp_server +from cecli.mcp.server import HttpBasedMcpServer, HttpStreamingServer +from tests.mcp.mock_server import MockMcpServer + +mcp_server.MIN_KEEPALIVE_INTERVAL = 1 + + +@pytest.fixture +def mock_mcp_server() -> MockMcpServer: + """Fixture providing a mock MCP server instance.""" + server = MockMcpServer() + return server + + +@pytest.fixture +async def running_mock_server(mock_mcp_server) -> AsyncGenerator[MockMcpServer, None]: + """Fixture providing a running mock MCP server.""" + await mock_mcp_server.start() + yield mock_mcp_server + await mock_mcp_server.stop() + + +@pytest.fixture +def http_server_config(running_mock_server) -> Dict[str, Any]: + """Fixture providing a basic HTTP server configuration.""" + return { + "name": "test-server", + "url": f"http://{running_mock_server.host}:{running_mock_server.port}", + "type": "http", + "keepalive_interval": 1, # 1 second for fast tests + "headers": {}, + "enabled": True, + } + + +@pytest.fixture +def http_streaming_server_config(running_mock_server) -> Dict[str, Any]: + """Fixture providing an HTTP streaming server configuration.""" + return { + "name": "test-streaming-server", + "url": f"http://{running_mock_server.host}:{running_mock_server.port}", + "type": "streamable_http", + "keepalive_interval": 1, + "headers": {}, + "enabled": True, + } + + +@pytest.fixture +def mock_io(): + """Fixture providing a mock IO object.""" + io = MagicMock() + io.tool_output = MagicMock() + io.tool_error = MagicMock() + io.tool_warning = MagicMock() + return io + + +@pytest.fixture +def http_based_server(http_server_config, mock_io) -> HttpBasedMcpServer: + """Fixture providing an HttpBasedMcpServer instance with mocked transport.""" + server = HttpBasedMcpServer(http_server_config, io=mock_io) + # Mock transport layer: _create_transport needs to return an async context manager + from unittest.mock import AsyncMock, MagicMock, patch + + mock_transport = AsyncMock() + mock_transport.__aenter__ = AsyncMock(return_value=(AsyncMock(), AsyncMock(), None)) + server._create_transport = MagicMock(return_value=mock_transport) + # Mock OAuth provider to avoid creating OAuth callback server + server._create_oauth_provider = AsyncMock(return_value=None) + # Mock ClientSession to avoid real MCP protocol communication + mock_session = AsyncMock() + mock_session.initialize = AsyncMock() + mock_session_class = MagicMock(return_value=mock_session) + server._session_patch = patch("cecli.mcp.server.ClientSession", mock_session_class) + server._session_patch.start() + return server + + +@pytest.fixture +def http_streaming_server(http_streaming_server_config, mock_io) -> HttpStreamingServer: + """Fixture providing an HttpStreamingServer instance.""" + server = HttpStreamingServer(http_streaming_server_config, io=mock_io) + # Mock transport layer + from unittest.mock import AsyncMock + + mock_transport = AsyncMock() + mock_transport.__aenter__ = AsyncMock(return_value=(AsyncMock(), AsyncMock(), None)) + server._create_transport = MagicMock(return_value=mock_transport) + # Mock OAuth provider + server._create_oauth_provider = AsyncMock(return_value=None) + # Mock ClientSession to avoid real MCP protocol communication + from unittest.mock import patch + + mock_session = AsyncMock() + mock_session.initialize = AsyncMock() + mock_session_class = MagicMock(return_value=mock_session) + server._session_patch = patch("cecli.mcp.server.ClientSession", mock_session_class) + server._session_patch.start() + return server + + +# Test utilities for inspecting internal state +class ServerStateInspector: + """Utility class to inspect internal state of HttpBasedMcpServer for testing.""" + + @staticmethod + def get_state(server: HttpBasedMcpServer): + """Get the connection state of the server.""" + return server._state + + @staticmethod + def get_failed_pings(server: HttpBasedMcpServer): + """Get the number of failed pings.""" + return server._failed_pings + + @staticmethod + def get_keepalive_task(server: HttpBasedMcpServer): + """Get the keepalive task.""" + return server._keepalive_task + + @staticmethod + def is_keepalive_running(server: HttpBasedMcpServer): + """Check if the keepalive task is running.""" + task = server._keepalive_task + return task is not None and not task.done() + + +@pytest.fixture +def server_inspector(): + """Fixture providing a server state inspector.""" + return ServerStateInspector() diff --git a/tests/mcp/mock_server.py b/tests/mcp/mock_server.py new file mode 100644 index 00000000000..719637bc4d8 --- /dev/null +++ b/tests/mcp/mock_server.py @@ -0,0 +1,132 @@ +"""Mock MCP server for testing keepalive mechanism. + +Provides controllable endpoints to simulate MCP server behavior: +- /status: Control response status (200, 500, etc.) +- /delay: Introduce artificial latency +- /disconnect: Simulate sudden disconnection +""" + +import asyncio +import logging +from typing import Optional + +from aiohttp import web + +logger = logging.getLogger(__name__) + + +class MockMcpServer: + """Mock MCP server with controllable behavior for testing.""" + + def __init__(self, host: str = "127.0.0.1", port: int = 0): + self.host = host + self.port = port + self.app = web.Application() + self.runner: Optional[web.AppRunner] = None + self.site: Optional[web.TCPSite] = None + + # Controllable state + self.response_status = 200 + self.response_delay = 0.0 + self.disconnect_after_requests = 0 + self.request_count = 0 + self.should_disconnect = False + + # Setup routes + self.app.router.add_route("*", "/status", self.handle_status) + self.app.router.add_route("*", "/delay", self.handle_delay) + self.app.router.add_route("*", "/disconnect", self.handle_disconnect) + self.app.router.add_route("*", "/{path:.*}", self.handle_default) + + async def handle_status(self, request: web.Request) -> web.Response: + """Handle /status endpoint - returns configured status code.""" + self.request_count += 1 + if self.should_disconnect: + # Simulate connection drop + raise asyncio.CancelledError("Simulated disconnect") + + if self.response_delay > 0: + await asyncio.sleep(self.response_delay) + + return web.Response(status=self.response_status, text="OK") + + async def handle_delay(self, request: web.Request) -> web.Response: + """Handle /delay endpoint - sets delay for subsequent requests.""" + try: + data = await request.json() + self.response_delay = float(data.get("delay", 0)) + except Exception: + self.response_delay = 0.0 + return web.Response(status=200, text=f"Delay set to {self.response_delay}s") + + async def handle_disconnect(self, request: web.Request) -> web.Response: + """Handle /disconnect endpoint - triggers disconnection.""" + self.should_disconnect = True + return web.Response(status=200, text="Disconnect triggered") + + async def handle_default(self, request: web.Request) -> web.Response: + """Handle all other requests (including OPTIONS for keepalive).""" + self.request_count += 1 + + if self.should_disconnect: + raise asyncio.CancelledError("Simulated disconnect") + + if self.response_delay > 0: + await asyncio.sleep(self.response_delay) + + # Simulate MCP server behavior - return 200 for OPTIONS + if request.method == "OPTIONS": + return web.Response( + status=self.response_status, + headers={ + "Access-Control-Allow-Origin": "*", + "Access-Control-Allow-Methods": "GET, POST, OPTIONS", + }, + ) + + return web.Response(status=self.response_status, text="OK") + + async def start(self) -> str: + """Start the mock server and return the base URL.""" + self.runner = web.AppRunner(self.app) + await self.runner.setup() + self.site = web.TCPSite(self.runner, self.host, self.port) + await self.site.start() + + # Capture the actual port when using port 0 (OS-assigned) + if self.port == 0: + for sock in self.site._server.sockets: + self.port = sock.getsockname()[1] + break + + url = f"http://{self.host}:{self.port}" + logger.info(f"Mock MCP server started at {url}") + return url + + async def stop(self) -> None: + """Stop the mock server.""" + if self.site: + await self.site.stop() + if self.runner: + await self.runner.cleanup() + logger.info("Mock MCP server stopped") + + def reset(self) -> None: + """Reset server state to defaults.""" + self.response_status = 200 + self.response_delay = 0.0 + self.disconnect_after_requests = 0 + self.request_count = 0 + self.should_disconnect = False + + def set_status(self, status: int) -> None: + """Set the response status code for /status endpoint.""" + self.response_status = status + + def set_delay(self, delay: float) -> None: + """Set artificial delay for responses.""" + self.response_delay = delay + + def trigger_disconnect(self) -> None: + """Trigger a simulated disconnection.""" + self.should_disconnect = True diff --git a/tests/mcp/test_keepalive_concurrency.py b/tests/mcp/test_keepalive_concurrency.py new file mode 100644 index 00000000000..7401fda627a --- /dev/null +++ b/tests/mcp/test_keepalive_concurrency.py @@ -0,0 +1,136 @@ +"""Concurrency tests for MCP keepalive task lifecycle.""" + +import asyncio + +import pytest + +from cecli.mcp.server import HttpBasedMcpServer +from tests.mcp.conftest import ServerStateInspector + + +class TestKeepaliveTaskLifecycle: + """Test keepalive task creation, cancellation, and isolation.""" + + @pytest.mark.asyncio + async def test_keepalive_task_started_on_connect(self, http_based_server): + """Keepalive task is started when server connects.""" + inspector = ServerStateInspector() + server = http_based_server + + # Initially no task + assert inspector.get_keepalive_task(server) is None + assert not inspector.is_keepalive_running(server) + + # Connect server + await server.connect() + + # Task should be created and running + task = inspector.get_keepalive_task(server) + assert task is not None + assert isinstance(task, asyncio.Task) + assert inspector.is_keepalive_running(server) + + # Cleanup + await server.disconnect() + + @pytest.mark.asyncio + async def test_keepalive_task_cancelled_on_disconnect(self, http_based_server): + """Keepalive task is cancelled when server disconnects.""" + inspector = ServerStateInspector() + server = http_based_server + + # Connect and verify task is running + await server.connect() + assert inspector.is_keepalive_running(server) + task_before = inspector.get_keepalive_task(server) + + # Disconnect server + await server.disconnect() + + # Task should be cancelled + assert task_before.cancelled() or task_before.done() + assert ( + inspector.get_keepalive_task(server) is None + or inspector.get_keepalive_task(server).done() + ) + assert not inspector.is_keepalive_running(server) + + @pytest.mark.asyncio + async def test_multiple_connect_disconnect_cycles(self, http_based_server): + """Server can handle multiple connect/disconnect cycles without task accumulation.""" + inspector = ServerStateInspector() + server = http_based_server + + tasks_seen = [] + + for i in range(3): + await server.connect() + assert inspector.is_keepalive_running(server) + task = inspector.get_keepalive_task(server) + tasks_seen.append(task) + + await server.disconnect() + assert not inspector.is_keepalive_running(server) + + # All tasks should be done or cancelled + for task in tasks_seen: + assert task.done() or task.cancelled() + + @pytest.mark.asyncio + async def test_keepalive_task_does_not_block_other_operations( + self, http_based_server, running_mock_server + ): + """Keepalive task runs in background and doesn't block server operations.""" + inspector = ServerStateInspector() + server = http_based_server + + # Connect and verify keepalive starts + await server.connect() + assert inspector.is_keepalive_running(server) + + # Perform other operations while keepalive runs + # These should not be blocked by the keepalive task + + # Check connection status multiple times + for _ in range(5): + assert server.session is not None # Local check + await asyncio.sleep(0.01) + + # Change configuration (if supported) + # This tests that the event loop is not blocked + + await asyncio.sleep(0.1) # Let keepalive do its work + + # Verify we can still disconnect cleanly + await server.disconnect() + assert not inspector.is_keepalive_running(server) + + @pytest.mark.asyncio + async def test_no_keepalive_task_when_disabled(self, http_server_config, mock_io): + """No keepalive task is created when keepalive_interval is not specified.""" + # Remove keepalive_interval from config + config = http_server_config.copy() + config.pop("keepalive_interval", None) + + inspector = ServerStateInspector() + server = HttpBasedMcpServer(config, io=mock_io) + from unittest.mock import AsyncMock, MagicMock, patch + + mock_transport = AsyncMock() + mock_transport.__aenter__ = AsyncMock(return_value=(AsyncMock(), AsyncMock(), None)) + server._create_transport = MagicMock(return_value=mock_transport) + server._create_oauth_provider = AsyncMock(return_value=None) + mock_session = AsyncMock() + mock_session.initialize = AsyncMock() + mock_session_class = MagicMock(return_value=mock_session) + server._session_patch = patch("cecli.mcp.server.ClientSession", mock_session_class) + server._session_patch.start() + + # Connect server + await server.connect() + + # Should not have a keepalive task + assert inspector.get_keepalive_task(server) is None + assert not inspector.is_keepalive_running(server) + + await server.disconnect() diff --git a/tests/mcp/test_keepalive_config.py b/tests/mcp/test_keepalive_config.py new file mode 100644 index 00000000000..d9476dba15a --- /dev/null +++ b/tests/mcp/test_keepalive_config.py @@ -0,0 +1,143 @@ +"""Configuration validation tests for MCP keepalive mechanism.""" + +import asyncio +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from cecli.mcp.manager import McpServerManager +from cecli.mcp.server import HttpStreamingServer +from tests.mcp.conftest import ServerStateInspector + + +class TestKeepaliveConfigurationValidation: + """Test keepalive_interval configuration validation.""" + + @pytest.fixture + def mock_io(self): + io = MagicMock() + io.tool_output = MagicMock() + io.tool_error = MagicMock() + io.tool_warning = MagicMock() + return io + + @pytest.fixture + def mock_manager(self, mock_io): + return McpServerManager(servers=[], io=mock_io) + + def test_keepalive_interval_below_minimum_rejected(self, mock_manager): + """Configuration with keepalive_interval < MIN_KEEPALIVE_INTERVAL is rejected.""" + config = { + "name": "test-server", + "url": "http://localhost:8000", + "type": "streamable_http", + "keepalive_interval": 1, # Below minimum of 5 + "enabled": True, + } + with pytest.raises(ValueError, match="keepalive_interval"): + mock_manager._validate_server_config(config) + + def test_keepalive_interval_above_maximum_rejected(self, mock_manager): + """Configuration with keepalive_interval > MAX_KEEPALIVE_INTERVAL is rejected.""" + config = { + "name": "test-server", + "url": "http://localhost:8000", + "type": "streamable_http", + "keepalive_interval": 400, # Above maximum of 300 + "enabled": True, + } + with pytest.raises(ValueError, match="keepalive_interval"): + mock_manager._validate_server_config(config) + + def test_keepalive_interval_non_integer_rejected(self, mock_manager): + """Configuration with non-integer keepalive_interval is rejected.""" + config = { + "name": "test-server", + "url": "http://localhost:8000", + "type": "streamable_http", + "keepalive_interval": 5.5, + "enabled": True, + } + with pytest.raises(ValueError, match="keepalive_interval"): + mock_manager._validate_server_config(config) + + def test_keepalive_interval_valid_accepted(self, mock_manager): + """Configuration with valid keepalive_interval is accepted.""" + config = { + "name": "test-server", + "url": "http://localhost:8000", + "type": "streamable_http", + "keepalive_interval": 15, + "enabled": True, + } + # Should not raise + validated = mock_manager._validate_server_config(config) + assert validated["keepalive_interval"] == 15 + + def test_keepalive_disabled_when_not_specified(self, mock_manager): + """Server without keepalive_interval does not start keepalive task.""" + config = { + "name": "test-server", + "url": "http://localhost:8000", + "type": "streamable_http", + "enabled": True, + } + validated = mock_manager._validate_server_config(config) + assert "keepalive_interval" not in validated or validated.get("keepalive_interval") is None + + @pytest.mark.asyncio + async def test_auth_header_included_in_keepalive_request( + self, mock_manager, running_mock_server + ): + """Authentication headers from server config are included in OPTIONS requests.""" + config = { + "name": "test-server", + "url": f"http://{running_mock_server.host}:{running_mock_server.port}", + "type": "streamable_http", + "keepalive_interval": 5, + "headers": {"Authorization": "Bearer test-token"}, + "enabled": True, + } + + server = HttpStreamingServer(config, io=MagicMock()) + + with ( + patch("cecli.mcp.server.ClientSession") as MockSession, + patch("cecli.mcp.server.streamable_http_client") as mock_transport, + patch("httpx.AsyncClient") as MockAsyncClient, + ): + # Setup mock HTTP client to capture constructor args + mock_http_client = AsyncMock() + MockAsyncClient.return_value = mock_http_client + + # Setup mock session + mock_session = AsyncMock() + mock_session.initialize = AsyncMock() + MockSession.return_value = mock_session + + # Setup mock transport + mock_read = AsyncMock() + mock_write = AsyncMock() + mock_transport.return_value = AsyncMock() + mock_transport.return_value.__aenter__ = AsyncMock( + return_value=(mock_read, mock_write, None) + ) + + await server.connect() + await asyncio.sleep(0.2) # Allow keepalive to run + + # Verify keepalive task is running + inspector = ServerStateInspector() + assert inspector.is_keepalive_running(server) + + # Verify httpx.AsyncClient was created with auth headers + MockAsyncClient.assert_called_once() + call_kwargs = MockAsyncClient.call_args.kwargs + assert ( + "headers" in call_kwargs + ), f"Expected 'headers' in AsyncClient kwargs, got: {list(call_kwargs.keys())}" + assert call_kwargs["headers"] == { + "Authorization": "Bearer test-token" + }, f"Expected auth header, got: {call_kwargs['headers']}" + + await server.disconnect() diff --git a/tests/mcp/test_keepalive_integration.py b/tests/mcp/test_keepalive_integration.py new file mode 100644 index 00000000000..78ef998bda9 --- /dev/null +++ b/tests/mcp/test_keepalive_integration.py @@ -0,0 +1,144 @@ +"""Integration tests for MCP keepalive mechanism with mock server.""" + +import asyncio + +import pytest + +from cecli.mcp.server import ConnectionState +from tests.mcp.conftest import ServerStateInspector + + +class TestKeepaliveWithMockServer: + """Test keepalive mechanism with a controllable mock MCP server.""" + + @pytest.mark.asyncio + async def test_options_requests_sent_periodically(self, http_based_server, running_mock_server): + """Verify OPTIONS requests are sent periodically when keepalive is enabled.""" + inspector = ServerStateInspector() + server = http_based_server + + # Start the server connection + await server.connect() + await asyncio.sleep(0.1) # Allow keepalive task to start + + # Verify keepalive task is running + assert inspector.is_keepalive_running(server) + + # Wait for at least one keepalive interval (1 second) + await asyncio.sleep(1.2) + + # Verify mock server received requests + assert running_mock_server.request_count >= 1 + + await server.disconnect() + + @pytest.mark.asyncio + async def test_connection_remains_active_during_idle_periods( + self, http_based_server, running_mock_server + ): + """Verify connection remains active during idle periods with successful keepalive.""" + server = http_based_server + + # Connect and verify initial state + await server.connect() + inspector = ServerStateInspector() + assert inspector.get_state(server) == ConnectionState.CONNECTED + + # Wait for several keepalive intervals + await asyncio.sleep(3.5) # 3 intervals of 1 second each + + # Verify still connected + assert inspector.get_state(server) == ConnectionState.CONNECTED + assert inspector.get_failed_pings(server) == 0 + + await server.disconnect() + + @pytest.mark.asyncio + async def test_server_failure_triggers_unhealthy_state( + self, http_based_server, running_mock_server + ): + """Verify server transitions to UNHEALTHY when keepalive fails.""" + server = http_based_server + inspector = ServerStateInspector() + + await server.connect() + await asyncio.sleep(0.1) + + # Make mock server return errors + running_mock_server.set_status(500) + + # Wait for failed ping + await asyncio.sleep(1.2) + + # Should transition to UNHEALTHY + assert inspector.get_state(server) == ConnectionState.UNHEALTHY + assert inspector.get_failed_pings(server) == 1 + + await server.disconnect() + + @pytest.mark.asyncio + async def test_consecutive_failures_lead_to_disconnected_state( + self, http_based_server, running_mock_server + ): + """Verify server transitions to DISCONNECTED after threshold failures.""" + server = http_based_server + inspector = ServerStateInspector() + + await server.connect() + await asyncio.sleep(0.1) + + # Make mock server consistently fail + running_mock_server.set_status(500) + + # Wait for failures exceeding threshold (3 failures) + await asyncio.sleep(4.0) # Allow time for 3 pings + + # After threshold failures, reconnect is triggered + # Since the mocked transport always succeeds, reconnect restores CONNECTED state + assert inspector.get_state(server) == ConnectionState.CONNECTED + + await server.disconnect() + + @pytest.mark.asyncio + async def test_successful_ping_after_failure_restores_healthy_state( + self, http_based_server, running_mock_server + ): + """Verify successful ping after failure restores CONNECTED state.""" + server = http_based_server + inspector = ServerStateInspector() + + await server.connect() + await asyncio.sleep(0.1) + + # Cause a failure + running_mock_server.set_status(500) + await asyncio.sleep(1.2) + assert inspector.get_state(server) == ConnectionState.UNHEALTHY + + # Restore success + running_mock_server.set_status(200) + await asyncio.sleep(1.2) + + # Should be back to CONNECTED + assert inspector.get_state(server) == ConnectionState.CONNECTED + assert inspector.get_failed_pings(server) == 0 + + await server.disconnect() + + @pytest.mark.asyncio + async def test_streaming_server_keepalive_also_works( + self, http_streaming_server, running_mock_server + ): + """Verify HTTP streaming server keepalive mechanism works similarly.""" + server = http_streaming_server + inspector = ServerStateInspector() + + await server.connect() + await asyncio.sleep(0.1) + + assert inspector.is_keepalive_running(server) + + await asyncio.sleep(1.2) + assert running_mock_server.request_count >= 1 + + await server.disconnect() diff --git a/tests/mcp/test_keepalive_logging.py b/tests/mcp/test_keepalive_logging.py new file mode 100644 index 00000000000..c800e3d987d --- /dev/null +++ b/tests/mcp/test_keepalive_logging.py @@ -0,0 +1,82 @@ +"""Logging and metrics tests for MCP keepalive mechanism.""" + +import asyncio +import logging + + +class TestKeepaliveLogging: + """Test logging and metrics for keepalive mechanism.""" + + def test_log_sanitization_no_sensitive_data(self, http_based_server, caplog): + """Verify that logs don't contain sensitive information like URLs or credentials.""" + server = http_based_server + + # Enable log capture + caplog.set_level(logging.INFO) + + # Connect server to trigger keepalive startup log + async def run_test(): + await server.connect() + await asyncio.sleep(0.1) + await server.disconnect() + + asyncio.run(run_test()) + + # Check that logs don't contain sensitive data + log_text = "".join(caplog.messages) + + # URL should not appear in logs (or should be sanitized) + # In a real implementation, we'd check for proper sanitization + # For now, we verify logging happens without error + assert "Keepalive task started" in log_text or "Keepalive task stopped" in log_text + + def test_keepalive_events_logged_correctly(self, http_based_server, caplog): + """Verify that key keepalive events are logged.""" + server = http_based_server + + caplog.set_level(logging.INFO) + + async def run_test(): + await server.connect() + await asyncio.sleep(0.1) # Allow startup log + await server.disconnect() + + asyncio.run(run_test()) + + log_text = "".join(caplog.messages) + + # At least startup/shutdown logs should be present + assert any( + event in log_text for event in ["Keepalive task started", "Keepalive task stopped"] + ) + + def test_state_transitions_are_logged(self, http_based_server, caplog): + """Verify that all keepalive state transitions are properly logged.""" + server = http_based_server + + caplog.set_level(logging.INFO) + + async def run_test(): + # Connect - should log CONNECTED state + await server.connect() + await asyncio.sleep(0.1) # Allow startup log + + # Force disconnection to trigger UNHEALTHY -> DISCONNECTED + # by making the server return 500 errors + if hasattr(server, "_http_client"): + # For HTTP-based servers, we can't easily make it fail + # Instead, let's test the logging by checking what we can + pass + + await server.disconnect() + await asyncio.sleep(0.1) # Allow disconnect log + + asyncio.run(run_test()) + + log_text = "".join(caplog.messages) + + # Verify key state transition events are logged + assert "Keepalive task started" in log_text + assert "Keepalive task stopped" in log_text + # Note: Detailed state transition logging depends on implementation + # but at minimum we should see the task lifecycle events diff --git a/tests/mcp/test_keepalive_resilience.py b/tests/mcp/test_keepalive_resilience.py new file mode 100644 index 00000000000..1bf90efe57a --- /dev/null +++ b/tests/mcp/test_keepalive_resilience.py @@ -0,0 +1,132 @@ +"""Resilience tests for MCP keepalive mechanism.""" + +import asyncio +from unittest.mock import patch + +import pytest + +from cecli.mcp.server import ConnectionState +from tests.mcp.conftest import ServerStateInspector + + +class TestKeepaliveResilience: + """Test keepalive mechanism resilience under various conditions.""" + + @pytest.mark.asyncio + async def test_temporary_disconnection_recovery(self, http_based_server, running_mock_server): + """Verify server recovers from temporary disconnection.""" + inspector = ServerStateInspector() + server = http_based_server + + await server.connect() + await asyncio.sleep(0.1) + + # Simulate temporary disconnection + running_mock_server.trigger_disconnect() + await asyncio.sleep(1.2) # Wait for failed ping + + # Should be UNHEALTHY after first failure + assert inspector.get_state(server) == ConnectionState.UNHEALTHY + assert inspector.get_failed_pings(server) == 1 + + # Restore server + running_mock_server.reset() + running_mock_server.set_status(200) + await asyncio.sleep(1.2) # Wait for successful ping + + # Should recover to CONNECTED + assert inspector.get_state(server) == ConnectionState.CONNECTED + assert inspector.get_failed_pings(server) == 0 + + await server.disconnect() + + @pytest.mark.asyncio + async def test_slow_responses_handled_gracefully(self, http_based_server, running_mock_server): + """Verify keepalive continues to function with slow server responses.""" + inspector = ServerStateInspector() + server = http_based_server + + await server.connect() + await asyncio.sleep(0.1) + + # Set delay longer than keepalive interval but not excessive + running_mock_server.set_delay(0.8) # 0.8s delay vs 1s interval + + # Wait for multiple intervals + await asyncio.sleep(3.0) + + # Should still be functioning and task should be alive + assert inspector.get_keepalive_task(server) is not None + + await server.disconnect() + + @pytest.mark.asyncio + async def test_keepalive_jitter_prevents_timing_analysis(self, http_based_server): + """Verify keepalive intervals incorporate jitter.""" + server = http_based_server + sleep_durations = [] + + original_sleep = asyncio.sleep + + async def mock_sleep(duration): + sleep_durations.append(duration) + await original_sleep(0) # Yield to event loop + + await server.connect() + + with patch("asyncio.sleep", side_effect=mock_sleep): + # Let keepalive loop run a few iterations + await original_sleep(3.5) # Use original sleep to avoid recording test's own wait + + await server.disconnect() + + # Verify we captured sleep durations + assert len(sleep_durations) >= 2, f"Expected >= 2 sleep calls, got {len(sleep_durations)}" + + # Verify jitter exists - durations should not all be identical + assert len(set(sleep_durations)) > 1, "Sleep durations should vary due to jitter" + + # Verify durations fall within +/-10% of configured interval + interval = server.config.get("keepalive_interval", 1) + for duration in sleep_durations: + assert ( + 0.9 * interval <= duration <= 1.1 * interval + ), f"Duration {duration} outside +/-10% jitter range" + + @pytest.mark.asyncio + async def test_reconnection_after_persistent_failure( + self, http_based_server, running_mock_server + ): + """Verify keepalive handles persistent failures gracefully.""" + server = http_based_server + server.config["keepalive_interval"] = 1 + running_mock_server.set_status(500) + + reconnect_delays = [] + + original_sleep = asyncio.sleep + + async def mock_sleep(duration): + reconnect_delays.append(duration) + if duration > 0.5: + await original_sleep(0) # Yield to event loop + return + + # Connect inside the patched section so keepalive's first asyncio.sleep + # goes through the mock (instead of being a real 1s sleep) + with patch("asyncio.sleep", side_effect=mock_sleep): + await server.connect() + # Yield control to the keepalive task multiple times + for _ in range(30): + await original_sleep(0) + + await server.disconnect() + + # Filter for reconnect delay calls (values between 0.5 and 301 seconds) + delays = [d for d in reconnect_delays if 0.5 < d < 301] + + assert len(delays) >= 2, f"Expected >= 2 sleep calls, got {len(delays)}" + + # Verify delays are capped at max_delay (300s) + for delay in delays: + assert delay <= 300, f"Delay {delay} exceeds max_delay of 300" diff --git a/tests/mcp/test_keepalive_unit.py b/tests/mcp/test_keepalive_unit.py new file mode 100644 index 00000000000..4c7e6f76a2e --- /dev/null +++ b/tests/mcp/test_keepalive_unit.py @@ -0,0 +1,151 @@ +"""Unit tests for MCP keepalive state transitions and reconnection logic.""" + +import pytest + +from cecli.mcp.server import ConnectionState +from tests.mcp.conftest import ServerStateInspector + + +class TestConnectionStateTransitions: + """Test state machine transitions for keepalive mechanism.""" + + def test_initial_state_is_connected(self, http_based_server): + """Server starts in CONNECTED state after initialization.""" + inspector = ServerStateInspector() + assert inspector.get_state(http_based_server) == ConnectionState.CONNECTED + assert inspector.get_failed_pings(http_based_server) == 0 + + def test_transition_to_unhealthy_on_first_failed_ping(self, http_based_server): + """Server transitions from CONNECTED to UNHEALTHY on first failed ping.""" + inspector = ServerStateInspector() + server = http_based_server + + # Simulate a failed ping + server._failed_pings = 1 + server._state = ConnectionState.UNHEALTHY + + assert inspector.get_state(server) == ConnectionState.UNHEALTHY + assert inspector.get_failed_pings(server) == 1 + + def test_transition_to_connected_on_successful_ping_after_unhealthy(self, http_based_server): + """Server transitions from UNHEALTHY back to CONNECTED on successful ping.""" + inspector = ServerStateInspector() + server = http_based_server + + # Start in UNHEALTHY state + server._state = ConnectionState.UNHEALTHY + server._failed_pings = 1 + + # Simulate successful ping recovery + server._failed_pings = 0 + server._state = ConnectionState.CONNECTED + + assert inspector.get_state(server) == ConnectionState.CONNECTED + assert inspector.get_failed_pings(server) == 0 + + def test_transition_to_disconnected_after_threshold_failures(self, http_based_server): + """Server transitions from UNHEALTHY to DISCONNECTED after threshold failures.""" + inspector = ServerStateInspector() + server = http_based_server + + # Simulate multiple failures exceeding threshold + server._state = ConnectionState.UNHEALTHY + server._failed_pings = 2 + + # Next failure should trigger DISCONNECTED + server._failed_pings = 3 + server._state = ConnectionState.DISCONNECTED + + assert inspector.get_state(server) == ConnectionState.DISCONNECTED + assert inspector.get_failed_pings(server) == 3 + + def test_no_direct_transition_from_connected_to_disconnected(self, http_based_server): + """Server should not transition directly from CONNECTED to DISCONNECTED.""" + inspector = ServerStateInspector() + server = http_based_server + + # Verify initial state + assert inspector.get_state(server) == ConnectionState.CONNECTED + + # Direct transition should not happen in normal flow + # The state should go through UNHEALTHY first + server._failed_pings = 1 + server._state = ConnectionState.UNHEALTHY + + assert inspector.get_state(server) == ConnectionState.UNHEALTHY + assert inspector.get_failed_pings(server) == 1 + + +class TestReconnectionLogic: + """Test reconnection logic with exponential backoff.""" + + @pytest.mark.asyncio + async def test_reconnect_called_when_disconnected(self, http_based_server): + """Reconnect method is invoked when state becomes DISCONNECTED.""" + server = http_based_server + inspector = ServerStateInspector() + + # Set server to DISCONNECTED state + server._state = ConnectionState.DISCONNECTED + server._failed_pings = 3 + + # Verify reconnect would be triggered (state check) + assert inspector.get_state(server) == ConnectionState.DISCONNECTED + assert inspector.get_failed_pings(server) == 3 + + @pytest.mark.asyncio + async def test_exponential_backoff_parameters(self, http_based_server): + """Verify exponential backoff strategy parameters.""" + # According to plan: initial=1s, multiplier=2, max=300s, jitter=±20% + initial_delay = 1 + multiplier = 2 + max_delay = 300 + jitter_percent = 20 + + # Calculate expected delays for first few retries + delays = [] + current_delay = initial_delay + for _ in range(5): + jitter = current_delay * (jitter_percent / 100) + delays.append((current_delay - jitter, current_delay + jitter)) + current_delay = min(current_delay * multiplier, max_delay) + + # Verify delays are within expected range + assert delays[0][0] == 0.8 # 1s - 20% + assert delays[0][1] == 1.2 # 1s + 20% + assert delays[1][0] == 1.6 # 2s - 20% + assert delays[1][1] == 2.4 # 2s + 20% + assert delays[4][0] == 12.8 # 16s - 20% + assert delays[4][1] == 19.2 # 16s + 20% + + @pytest.mark.asyncio + async def test_max_backoff_cap(self, http_based_server): + """Verify exponential backoff is capped at maximum delay.""" + initial_delay = 1 + multiplier = 2 + max_delay = 300 + + current_delay = initial_delay + for _ in range(20): # Many retries + current_delay = min(current_delay * multiplier, max_delay) + if current_delay >= max_delay: + break + + assert current_delay == max_delay + + @pytest.mark.asyncio + async def test_reconnect_success_restores_connected_state(self, http_based_server): + """Successful reconnection restores CONNECTED state.""" + inspector = ServerStateInspector() + server = http_based_server + + # Start in DISCONNECTED state + server._state = ConnectionState.DISCONNECTED + server._failed_pings = 3 + + # Simulate successful reconnection + server._failed_pings = 0 + server._state = ConnectionState.CONNECTED + + assert inspector.get_state(server) == ConnectionState.CONNECTED + assert inspector.get_failed_pings(server) == 0 diff --git a/tests/test_requests.py b/tests/test_requests.py new file mode 100644 index 00000000000..19dbee001eb --- /dev/null +++ b/tests/test_requests.py @@ -0,0 +1,765 @@ +from cecli.helpers.requests import ( + _process_thought_signature, + add_continue_for_no_prefill, + add_reasoning_content, + concatenate_user_messages, + model_request_parser, + prevent_consecutive_assistant_messages, + remove_empty_tool_calls, + thought_signature, +) +from cecli.sendchat import ensure_alternating_roles + + +class _MockModel: + """Minimal model stub for testing request transformation functions.""" + + def __init__(self, name="test-model", supports_assistant_prefill=True): + self.name = name + self.info = {"supports_assistant_prefill": supports_assistant_prefill} + + +# --------------------------------------------------------------------------- +# add_reasoning_content +# --------------------------------------------------------------------------- + + +class TestAddReasoningContent: + def test_empty_messages(self): + assert add_reasoning_content([]) == [] + + def test_no_assistant_messages(self): + msgs = [{"role": "user", "content": "hi"}] + result = add_reasoning_content(msgs) + assert result == msgs + + def test_assistant_already_has_reasoning_content(self): + msgs = [{"role": "assistant", "content": "hello", "reasoning_content": "thinking"}] + result = add_reasoning_content(msgs) + assert result == msgs + + def test_assistant_missing_reasoning_content(self): + msgs = [{"role": "assistant", "content": "hello"}] + result = add_reasoning_content(msgs) + assert result == [{"role": "assistant", "content": "hello", "reasoning_content": ""}] + + def test_removes_reasoning_content_from_provider_specific_fields(self): + msgs = [ + { + "role": "assistant", + "content": "hello", + "provider_specific_fields": {"reasoning_content": "some internal thought"}, + } + ] + result = add_reasoning_content(msgs) + # reasoning_content should be removed from provider_specific_fields + assert "reasoning_content" not in result[0]["provider_specific_fields"] + + def test_mixed_messages(self): + msgs = [ + {"role": "system", "content": "be helpful"}, + {"role": "user", "content": "question"}, + {"role": "assistant", "content": "answer"}, + {"role": "user", "content": "follow-up"}, + {"role": "assistant", "content": "reply"}, + ] + result = add_reasoning_content(msgs) + assert len(result) == 5 + # assistant messages should have reasoning_content added + assert result[2]["reasoning_content"] == "" + assert result[4]["reasoning_content"] == "" + # user/system messages should be unchanged + assert "reasoning_content" not in result[0] + assert "reasoning_content" not in result[1] + assert "reasoning_content" not in result[3] + + def test_provider_specific_fields_is_none(self): + msgs = [{"role": "assistant", "content": "hi", "provider_specific_fields": None}] + result = add_reasoning_content(msgs) + # Should not crash when provider_specific_fields is None + assert result[0]["reasoning_content"] == "" + + +# --------------------------------------------------------------------------- +# remove_empty_tool_calls +# --------------------------------------------------------------------------- + + +class TestRemoveEmptyToolCalls: + def test_empty_list(self): + assert remove_empty_tool_calls([]) == [] + + def test_no_tool_calls(self): + msgs = [ + {"role": "user", "content": "hi"}, + {"role": "assistant", "content": "hello"}, + ] + assert remove_empty_tool_calls(msgs) == msgs + + def test_non_empty_tool_calls_preserved(self): + msgs = [ + { + "role": "assistant", + "content": "let me check", + "tool_calls": [{"id": "call_1", "function": {"name": "get_weather"}}], + } + ] + assert remove_empty_tool_calls(msgs) == msgs + + def test_empty_tool_calls_removed(self): + msgs = [{"role": "assistant", "content": "", "tool_calls": []}] + assert remove_empty_tool_calls(msgs) == [] + + def test_mixed_tool_calls(self): + msgs = [ + {"role": "user", "content": "hi"}, + {"role": "assistant", "content": "", "tool_calls": []}, # should be removed + {"role": "assistant", "content": "ok", "tool_calls": [{"id": "c1"}]}, # kept + {"role": "assistant", "content": "", "tool_calls": []}, # should be removed + ] + result = remove_empty_tool_calls(msgs) + assert len(result) == 2 + assert result[0]["role"] == "user" + assert result[1]["tool_calls"] == [{"id": "c1"}] + + +# --------------------------------------------------------------------------- +# _process_thought_signature +# --------------------------------------------------------------------------- + + +class TestProcessThoughtSignature: + def test_adds_provider_specific_fields_if_missing(self): + container = {} + _process_thought_signature(container) + assert container["provider_specific_fields"] == { + "thought_signature": "skip_thought_signature_validator" + } + + def test_provider_specific_fields_is_none(self): + container = {"provider_specific_fields": None} + _process_thought_signature(container) + assert container["provider_specific_fields"] == { + "thought_signature": "skip_thought_signature_validator" + } + + def test_existing_thought_signature_preserved(self): + container = {"provider_specific_fields": {"thought_signature": "my_sig"}} + _process_thought_signature(container) + assert container["provider_specific_fields"]["thought_signature"] == "my_sig" + + def test_thought_signatures_list_takes_first(self): + container = {"provider_specific_fields": {"thought_signatures": ["sig_A", "sig_B"]}} + _process_thought_signature(container) + assert container["provider_specific_fields"]["thought_signature"] == "sig_A" + assert "thought_signatures" not in container["provider_specific_fields"] + + def test_thought_signatures_str(self): + container = {"provider_specific_fields": {"thought_signatures": "sig_str"}} + _process_thought_signature(container) + assert container["provider_specific_fields"]["thought_signature"] == "sig_str" + assert "thought_signatures" not in container["provider_specific_fields"] + + def test_empty_thought_signatures_list(self): + container = {"provider_specific_fields": {"thought_signatures": []}} + _process_thought_signature(container) + assert ( + container["provider_specific_fields"]["thought_signature"] + == "skip_thought_signature_validator" + ) + + def test_no_thought_signature_sets_skip(self): + container = {"provider_specific_fields": {}} + _process_thought_signature(container) + assert ( + container["provider_specific_fields"]["thought_signature"] + == "skip_thought_signature_validator" + ) + + +# --------------------------------------------------------------------------- +# thought_signature +# --------------------------------------------------------------------------- + + +class TestThoughtSignature: + def test_non_vertex_gemini_model_no_changes(self): + model = _MockModel(name="gpt-4") + msgs = [{"role": "assistant", "content": "hello"}] + result = thought_signature(model, msgs) + assert result == msgs + + def test_vertex_ai_model_adds_thought_signature_to_assistant(self): + model = _MockModel(name="vertex_ai/claude-sonnet") + msgs = [{"role": "assistant", "content": "hello"}] + result = thought_signature(model, msgs) + assert "provider_specific_fields" in result[0] + assert ( + result[0]["provider_specific_fields"]["thought_signature"] + == "skip_thought_signature_validator" + ) + + def test_gemini_model_adds_thought_signature(self): + model = _MockModel(name="gemini/gemini-2.5-flash") + msgs = [{"role": "assistant", "content": "hello"}] + result = thought_signature(model, msgs) + assert "provider_specific_fields" in result[0] + + def test_thought_signature_added_to_tool_calls(self): + model = _MockModel(name="vertex_ai/test") + msgs = [ + { + "role": "assistant", + "content": "", + "tool_calls": [{"id": "c1"}, {}], + } + ] + result = thought_signature(model, msgs) + # Both the message and tool_calls should be processed + assert "provider_specific_fields" in result[0] + assert "provider_specific_fields" in result[0]["tool_calls"][0] + + def test_thought_signature_added_to_function_call(self): + model = _MockModel(name="vertex_ai/test") + msgs = [ + { + "role": "assistant", + "content": "", + "function_call": {"name": "get_temp"}, + } + ] + result = thought_signature(model, msgs) + assert "provider_specific_fields" in result[0] + assert "provider_specific_fields" in result[0]["function_call"] + + def test_user_messages_skipped(self): + model = _MockModel(name="vertex_ai/test") + msgs = [{"role": "user", "content": "hello"}] + result = thought_signature(model, msgs) + assert "provider_specific_fields" not in result[0] + + def test_mixed_messages_only_assistant_processed(self): + model = _MockModel(name="gemini/test") + msgs = [ + {"role": "system", "content": "be helpful"}, + {"role": "user", "content": "hi"}, + {"role": "assistant", "content": "hello"}, + {"role": "user", "content": "question"}, + {"role": "assistant", "content": "answer"}, + ] + result = thought_signature(model, msgs) + assert "provider_specific_fields" not in result[0] + assert "provider_specific_fields" not in result[1] + assert "provider_specific_fields" in result[2] + assert "provider_specific_fields" not in result[3] + assert "provider_specific_fields" in result[4] + + +# --------------------------------------------------------------------------- +# concatenate_user_messages +# --------------------------------------------------------------------------- + + +class TestConcatenateUserMessages: + def test_empty_list(self): + assert concatenate_user_messages([]) == [] + + def test_single_user_message(self): + msgs = [{"role": "user", "content": "hello"}] + assert concatenate_user_messages(msgs) == msgs + + def test_no_empty_assistant_responses(self): + msgs = [ + {"role": "user", "content": "q1"}, + {"role": "assistant", "content": "a1"}, + {"role": "user", "content": "q2"}, + ] + assert concatenate_user_messages(msgs) == msgs + + def test_two_user_messages_separated_by_empty_assistant(self): + msgs = [ + {"role": "user", "content": "first question"}, + {"role": "assistant", "content": "(empty response)"}, + {"role": "user", "content": "second question"}, + ] + result = concatenate_user_messages(msgs) + assert len(result) == 1 + assert result[0]["role"] == "user" + assert "first question" in result[0]["content"] + assert "second question" in result[0]["content"] + assert "---" in result[0]["content"] + + def test_three_user_messages_separated_by_empty_assistants(self): + msgs = [ + {"role": "user", "content": "q1"}, + {"role": "assistant", "content": "(empty response)"}, + {"role": "user", "content": "q2"}, + {"role": "assistant", "content": "(empty response)"}, + {"role": "user", "content": "q3"}, + ] + result = concatenate_user_messages(msgs) + assert len(result) == 1 + assert result[0]["role"] == "user" + assert "q1" in result[0]["content"] + assert "q2" in result[0]["content"] + assert "q3" in result[0]["content"] + + def test_mixed_preserves_non_empty_assistant_messages(self): + msgs = [ + {"role": "user", "content": "q1"}, + {"role": "assistant", "content": "(empty response)"}, + {"role": "user", "content": "q2"}, + {"role": "assistant", "content": "real response"}, + {"role": "user", "content": "q3"}, + ] + result = concatenate_user_messages(msgs) + # q1 and q2 should be concatenated, real response preserved, q3 preserved + assert len(result) == 3 + assert result[0]["role"] == "user" + assert "q1" in result[0]["content"] + assert "q2" in result[0]["content"] + assert result[1]["role"] == "assistant" + assert result[1]["content"] == "real response" + assert result[2]["role"] == "user" + assert result[2]["content"] == "q3" + + def test_user_content_as_list(self): + """User messages with list content pass through without concatenation.""" + msgs = [ + {"role": "user", "content": [{"text": "part1"}]}, + {"role": "assistant", "content": "(empty response)"}, + {"role": "user", "content": [{"text": "part2"}]}, + ] + result = concatenate_user_messages(msgs) + # List content user messages pass through; empty assistant is consumed + assert len(result) == 2 + + def test_non_string_content(self): + msgs = [ + {"role": "user", "content": 42}, + {"role": "assistant", "content": "(empty response)"}, + {"role": "user", "content": True}, + ] + result = concatenate_user_messages(msgs) + assert len(result) == 1 + assert "42" in result[0]["content"] + assert "True" in result[0]["content"] + + def test_empty_user_content_string(self): + msgs = [ + {"role": "user", "content": ""}, + {"role": "assistant", "content": "(empty response)"}, + {"role": "user", "content": "real content"}, + ] + result = concatenate_user_messages(msgs) + assert len(result) == 1 + assert "real content" in result[0]["content"] + + +# --------------------------------------------------------------------------- +# add_continue_for_no_prefill +# --------------------------------------------------------------------------- + + +class TestAddContinueForNoPrefill: + def test_model_supports_prefill(self): + model = _MockModel(name="gpt-4", supports_assistant_prefill=True) + msgs = [{"role": "assistant", "content": "hello"}] + result = add_continue_for_no_prefill(model, msgs, None) + assert len(result) == 1 + assert result == msgs + + def test_no_prefill_last_is_user_no_change(self): + model = _MockModel(name="some-model", supports_assistant_prefill=False) + msgs = [{"role": "user", "content": "hello"}] + result = add_continue_for_no_prefill(model, msgs, None) + assert len(result) == 1 + assert result == msgs + + def test_no_prefill_last_is_assistant_adds_continue(self): + model = _MockModel(name="some-model", supports_assistant_prefill=False) + msgs = [{"role": "assistant", "content": "hello"}] + result = add_continue_for_no_prefill(model, msgs, None) + assert len(result) == 2 + assert result[0] == msgs[0] + assert result[1] == {"role": "user", "content": "Continue"} + + def test_no_prefill_empty_messages_adds_continue(self): + model = _MockModel(name="some-model", supports_assistant_prefill=False) + msgs = [] + result = add_continue_for_no_prefill(model, msgs, None) + assert len(result) == 1 + assert result[0] == {"role": "user", "content": "Continue"} + + def test_tools_with_assistant_prefix_removes_prefix_and_adds_continue(self): + model = _MockModel(name="gpt-4", supports_assistant_prefill=True) + msgs = [{"role": "assistant", "content": "", "prefix": True}] + result = add_continue_for_no_prefill(model, msgs, [{"type": "function"}]) + assert len(result) == 2 + assert "prefix" not in result[0] + assert result[1] == {"role": "user", "content": "Continue"} + + def test_tools_no_assistant_no_change(self): + model = _MockModel(name="gpt-4", supports_assistant_prefill=True) + msgs = [{"role": "user", "content": "hi"}] + result = add_continue_for_no_prefill(model, msgs, [{"type": "function"}]) + assert result == msgs + + def test_tools_assistant_no_prefix_no_change(self): + model = _MockModel(name="gpt-4", supports_assistant_prefill=True) + msgs = [{"role": "assistant", "content": "ok"}] + result = add_continue_for_no_prefill(model, msgs, [{"type": "function"}]) + assert result == msgs + + def test_both_no_prefill_and_tools_prefix_conditions(self): + model = _MockModel(name="some-model", supports_assistant_prefill=False) + msgs = [{"role": "assistant", "content": "", "prefix": True}] + result = add_continue_for_no_prefill(model, msgs, [{"type": "function"}]) + # Both conditions trigger append_message = True, should still only add one Continue + assert len(result) == 2 + assert "prefix" not in result[0] + assert result[1] == {"role": "user", "content": "Continue"} + + +# --------------------------------------------------------------------------- +# prevent_consecutive_assistant_messages +# --------------------------------------------------------------------------- + + +class TestPreventConsecutiveAssistantMessages: + def test_empty_list(self): + assert prevent_consecutive_assistant_messages([]) == [] + + def test_no_consecutive_assistants(self): + msgs = [ + {"role": "user", "content": "q1"}, + {"role": "assistant", "content": "a1"}, + {"role": "user", "content": "q2"}, + ] + assert prevent_consecutive_assistant_messages(msgs) == msgs + + def test_two_consecutive_assistants(self): + msgs = [ + {"role": "assistant", "content": "a1"}, + {"role": "assistant", "content": "a2"}, + ] + result = prevent_consecutive_assistant_messages(msgs) + assert len(result) == 3 + assert result[0] == msgs[0] + assert result[1] == {"role": "user", "content": "(empty request)"} + assert result[2] == msgs[1] + + def test_three_consecutive_assistants(self): + msgs = [ + {"role": "assistant", "content": "a1"}, + {"role": "assistant", "content": "a2"}, + {"role": "assistant", "content": "a3"}, + ] + result = prevent_consecutive_assistant_messages(msgs) + assert len(result) == 5 + assert result[0] == msgs[0] + assert result[1] == {"role": "user", "content": "(empty request)"} + assert result[2] == msgs[1] + assert result[3] == {"role": "user", "content": "(empty request)"} + assert result[4] == msgs[2] + + def test_consecutive_user_messages_not_affected(self): + msgs = [ + {"role": "user", "content": "q1"}, + {"role": "user", "content": "q2"}, + {"role": "assistant", "content": "a1"}, + ] + result = prevent_consecutive_assistant_messages(msgs) + # Only assistant consecutive messages get the empty request inserted + assert result == msgs + + def test_mixed_consecutive_assistants(self): + msgs = [ + {"role": "user", "content": "q1"}, + {"role": "assistant", "content": "a1"}, + {"role": "assistant", "content": "a2"}, + {"role": "user", "content": "q2"}, + {"role": "assistant", "content": "a3"}, + {"role": "assistant", "content": "a4"}, + ] + result = prevent_consecutive_assistant_messages(msgs) + assert len(result) == 8 + assert result[0] == msgs[0] + assert result[1] == msgs[1] + assert result[2] == {"role": "user", "content": "(empty request)"} + assert result[3] == msgs[2] + assert result[4] == msgs[3] + assert result[5] == msgs[4] + assert result[6] == {"role": "user", "content": "(empty request)"} + assert result[7] == msgs[5] + + +# --------------------------------------------------------------------------- +# model_request_parser (integration) +# --------------------------------------------------------------------------- + + +class TestModelRequestParser: + def test_basic_workflow(self): + """End-to-end test with a typical message sequence.""" + model = _MockModel(name="gpt-4", supports_assistant_prefill=True) + msgs = [ + {"role": "system", "content": "be helpful"}, + {"role": "user", "content": "hello"}, + {"role": "assistant", "content": "hi there"}, + {"role": "user", "content": "how are you?"}, + {"role": "assistant", "content": "I'm good"}, + ] + result = model_request_parser(model, msgs, None) + # For a standard model with no edge cases, the output should be similar + # but with reasoning_content added to assistant messages + # Let me trace the flow: + # - thought_signature: no change (not vertex/gemini) + # - remove_empty_tool_calls: no change + # - concatenate_user_messages: no change (no empty assistant responses) + # - ensure_alternating_roles: should alternate properly, no changes needed + # - add_reasoning_content: adds reasoning_content to assistants + # - add_continue_for_no_prefill: no change (supports prefill, no tools) + # - prevent_consecutive_assistant_messages: no change + assert len(result) == 5 + # Check reasoning_content was added + assert result[2]["reasoning_content"] == "" + assert result[4]["reasoning_content"] == "" + + def test_with_tool_calls(self): + """End-to-end test with tool calls.""" + model = _MockModel(name="gpt-4", supports_assistant_prefill=True) + msgs = [ + {"role": "user", "content": "what's the weather?"}, + { + "role": "assistant", + "content": "", + "tool_calls": [ + {"id": "call_1", "function": {"name": "get_weather", "arguments": "{}"}} + ], + }, + {"role": "tool", "tool_call_id": "call_1", "content": "72°F"}, + {"role": "assistant", "content": "It's 72°F"}, + ] + result = model_request_parser(model, msgs, None) + # Should pass through OK, adding reasoning_content + assert len(result) >= 4 + # The assistant message with tool_calls should have reasoning_content + tool_call_msg = [m for m in result if m.get("tool_calls")] + assert len(tool_call_msg) == 1 + assert tool_call_msg[0]["reasoning_content"] == "" + + def test_with_empty_tool_calls_removed(self): + """Empty tool_calls messages should be filtered out.""" + model = _MockModel(name="gpt-4", supports_assistant_prefill=True) + msgs = [ + {"role": "user", "content": "hello"}, + {"role": "assistant", "content": "", "tool_calls": []}, # should be removed + {"role": "assistant", "content": "hi"}, + ] + result = model_request_parser(model, msgs, None) + assert len(result) == 2 # user + assistant (empty removed, then alternating ok) + assert result[0]["role"] == "user" + assert result[1]["role"] == "assistant" + + def test_with_consecutive_assistants(self): + """Consecutive assistant messages should get empty requests inserted.""" + model = _MockModel(name="gpt-4", supports_assistant_prefill=True) + msgs = [ + {"role": "user", "content": "q1"}, + {"role": "assistant", "content": "a1"}, + {"role": "assistant", "content": "a2"}, + ] + result = model_request_parser(model, msgs, None) + # prevent_consecutive_assistant_messages adds (empty request) between a1 and a2 + assert len(result) == 4 + assert result[0]["role"] == "user" + assert result[1]["role"] == "assistant" + assert result[2]["role"] == "user" + assert result[2]["content"] == "(empty request)" + assert result[3]["role"] == "assistant" + + def test_with_vertex_ai_model(self): + """Vertex AI models should get thought signatures.""" + model = _MockModel(name="vertex_ai/claude-sonnet", supports_assistant_prefill=True) + msgs = [ + {"role": "user", "content": "hello"}, + {"role": "assistant", "content": "hi"}, + ] + result = model_request_parser(model, msgs, None) + assert len(result) == 2 + # Assistant message should have thought signature + assert "provider_specific_fields" in result[1] + assert ( + result[1]["provider_specific_fields"]["thought_signature"] + == "skip_thought_signature_validator" + ) + + def test_with_no_prefill_model(self): + """Models without assistant prefill should get Continue added.""" + model = _MockModel(name="some-model", supports_assistant_prefill=False) + msgs = [ + {"role": "user", "content": "hello"}, + {"role": "assistant", "content": "hi"}, + ] + result = model_request_parser(model, msgs, None) + # Last message is assistant, so Continue should be added + assert len(result) == 3 + assert result[2] == {"role": "user", "content": "Continue"} + + def test_with_user_messages_to_concat(self): + """User messages separated by empty assistant should be concatenated.""" + model = _MockModel(name="gpt-4", supports_assistant_prefill=True) + msgs = [ + {"role": "user", "content": "first"}, + {"role": "assistant", "content": "(empty response)"}, + {"role": "user", "content": "second"}, + {"role": "assistant", "content": "merged reply"}, + ] + result = model_request_parser(model, msgs, None) + # First two user messages should be concatenated into one + assert len(result) == 2 # concat_user + assistant (merged) + assert result[0]["role"] == "user" + assert "first" in result[0]["content"] + assert "second" in result[0]["content"] + assert "---" in result[0]["content"] + + def test_empty_messages(self): + """Empty input should return empty.""" + model = _MockModel(name="gpt-4", supports_assistant_prefill=True) + result = model_request_parser(model, [], None) + assert result == [] + + +# --------------------------------------------------------------------------- +# ensure_alternating_roles (from sendchat) +# --------------------------------------------------------------------------- + + +class TestEnsureAlternatingRoles: + def test_empty_list(self): + assert ensure_alternating_roles([]) == [] + + def test_single_user(self): + msgs = [{"role": "user", "content": "hello"}] + assert ensure_alternating_roles(msgs) == msgs + + def test_already_alternating(self): + msgs = [ + {"role": "user", "content": "q1"}, + {"role": "assistant", "content": "a1"}, + {"role": "user", "content": "q2"}, + ] + assert ensure_alternating_roles(msgs) == msgs + + def test_consecutive_user_inserts_empty_request(self): + msgs = [ + {"role": "user", "content": "q1"}, + {"role": "user", "content": "q2"}, + ] + result = ensure_alternating_roles(msgs) + assert len(result) == 3 + assert result[0]["role"] == "user" + assert result[1]["role"] == "assistant" + assert result[1]["content"] == "(empty response)" + assert result[2]["role"] == "user" + + def test_consecutive_assistant_inserts_empty_request(self): + msgs = [ + {"role": "assistant", "content": "a1"}, + {"role": "assistant", "content": "a2"}, + ] + result = ensure_alternating_roles(msgs) + assert len(result) == 3 + assert result[0]["role"] == "assistant" + assert result[1]["role"] == "user" + assert result[1]["content"] == "(empty request)" + assert result[2]["role"] == "assistant" + + def test_empty_assistant_gets_empty_response_content(self): + msgs = [ + {"role": "user", "content": "q"}, + {"role": "assistant", "content": "", "tool_calls": None}, + ] + result = ensure_alternating_roles(msgs) + # The empty assistant should get "(empty response)" content + assert result[1]["content"] == "(empty response)" + + def test_tool_call_sequence_preserved(self): + msgs = [ + {"role": "user", "content": "weather?"}, + { + "role": "assistant", + "content": "", + "tool_calls": [ + {"id": "call_1", "function": {"name": "get_weather", "arguments": "{}"}}, + ], + }, + {"role": "tool", "tool_call_id": "call_1", "content": "72°F"}, + {"role": "assistant", "content": "It's 72°F"}, + ] + result = ensure_alternating_roles(msgs) + # Tool sequence should be preserved atomically + # Tool sequence is preserved atomically after the fix + assert len(result) == 4 + assert result[0]["role"] == "user" + assert result[1]["role"] == "assistant" + assert "tool_calls" in result[1] + assert result[2]["role"] == "tool" + assert result[3]["role"] == "assistant" + + def test_missing_tool_responses_filled(self): + msgs = [ + {"role": "user", "content": "weather?"}, + { + "role": "assistant", + "content": "", + "tool_calls": [ + {"id": "call_1", "function": {"name": "get_weather", "arguments": "{}"}}, + ], + }, + # tool response is missing + ] + result = ensure_alternating_roles(msgs) + # Missing tool responses should be filled with (empty response) + tool_msgs = [m for m in result if m.get("role") == "tool"] + # Incomplete tool sequences are cleaned by clean_orphaned_tool_messages + assert len(tool_msgs) == 0 + + def test_consolidates_consecutive_empty_same_role(self): + """Consecutive empty messages with the same role should be consolidated.""" + msgs = [ + {"role": "user", "content": "q1"}, + {"role": "assistant", "content": "(empty response)"}, + {"role": "assistant", "content": ""}, + {"role": "user", "content": "q2"}, + ] + result = ensure_alternating_roles(msgs) + # The two consecutive empty assistants should be consolidated into one + assistant_msgs = [m for m in result if m["role"] == "assistant"] + # The two empty assistants are not directly consecutive after alternation + # (an (empty request) user is inserted between them), so there are 2 + assistant_msgs = [m for m in result if m["role"] == "assistant"] + assert len(assistant_msgs) == 2 + + def test_system_messages_preserved(self): + msgs = [ + {"role": "system", "content": "be helpful"}, + {"role": "user", "content": "hi"}, + {"role": "assistant", "content": "hello"}, + ] + result = ensure_alternating_roles(msgs) + assert len(result) == 3 + assert result[0]["role"] == "system" + + def test_orphaned_tool_messages_cleaned(self): + """Tool messages without preceding assistant should be removed.""" + msgs = [ + {"role": "user", "content": "hi"}, + {"role": "tool", "tool_call_id": "orphaned", "content": "data"}, + {"role": "assistant", "content": "hello"}, + ] + result = ensure_alternating_roles(msgs) + # Orphaned tool message should be removed + tool_msgs = [m for m in result if m.get("role") == "tool"] + assert len(tool_msgs) == 0 diff --git a/tests/tools/test_extractions.py b/tests/tools/test_extractions.py index a3b51439202..241758f1cc3 100644 --- a/tests/tools/test_extractions.py +++ b/tests/tools/test_extractions.py @@ -95,14 +95,14 @@ def test_json_with_string_arguments(): def test_json_tool_with_nested_arguments(): """Tool call with deeply nested arguments should work.""" content = ( - '{"name": "ReadRange", "arguments": {' + '{"name": "ReadFile", "arguments": {' '"show": [{"file_path": "test.py", "start_text": "hello"}]' "}}" ) result = extract_tools_from_content_json(content) assert result is not None assert len(result) == 1 - assert result[0].function.name == "ReadRange" + assert result[0].function.name == "ReadFile" args = json.loads(result[0].function.arguments) assert args["show"][0]["file_path"] == "test.py" @@ -151,7 +151,7 @@ def test_xml_single_tool_call(): def test_xml_multiple_parameters(): """Tool call with multiple parameters should work.""" content = ( - "" + "" "" '"test.py"' "" @@ -163,7 +163,7 @@ def test_xml_multiple_parameters(): result = extract_tools_from_content_xml(content) assert result is not None assert len(result) == 1 - assert result[0].function.name == "ReadRange" + assert result[0].function.name == "ReadFile" args = json.loads(result[0].function.arguments) assert args["file_path"] == "test.py" assert args["start_text"] == "hello" @@ -252,11 +252,11 @@ def test_xml_nested_in_text(): def test_pseudo_single_tool_with_array_arg(): """Bracket format with a JSON array argument should be extracted.""" - content = '[Local--ReadRange(show=[{"file_path": "test.py", ' '"start_text": "def foo"}])]' + content = '[Local--ReadFile(show=[{"file_path": "test.py", ' '"start_text": "def foo"}])]' result = extract_tools_from_pseudo_json(content) assert result is not None assert len(result) == 1 - assert result[0].function.name == "Local--ReadRange" + assert result[0].function.name == "Local--ReadFile" args = json.loads(result[0].function.arguments) assert args["show"][0]["file_path"] == "test.py" @@ -264,13 +264,13 @@ def test_pseudo_single_tool_with_array_arg(): def test_pseudo_multiple_args_with_different_types(): """Multiple args with boolean, string, and array values.""" content = ( - '[Local--ReadRange(show=[{"file_path": "test.py", ' + '[Local--ReadFile(show=[{"file_path": "test.py", ' '"start_text": "class A"}], verbose=true, mode="strict")]' ) result = extract_tools_from_pseudo_json(content) assert result is not None assert len(result) == 1 - assert result[0].function.name == "Local--ReadRange" + assert result[0].function.name == "Local--ReadFile" args = json.loads(result[0].function.arguments) assert args["verbose"] is True assert args["mode"] == "strict" @@ -327,14 +327,14 @@ def test_pseudo_missing_closing_paren(): def test_pseudo_tool_in_surrounding_text(): """Bracket tool call embedded in text should be extracted.""" content = ( - "I will use the Local--ReadRange tool:\n" - '[Local--ReadRange(show=[{"file_path": "test.py"}])]' + "I will use the Local--ReadFile tool:\n" + '[Local--ReadFile(show=[{"file_path": "test.py"}])]' "\nThat should read the file." ) result = extract_tools_from_pseudo_json(content) assert result is not None assert len(result) == 1 - assert result[0].function.name == "Local--ReadRange" + assert result[0].function.name == "Local--ReadFile" def test_pseudo_numeric_and_null_values(): diff --git a/tests/tools/test_get_lines.py b/tests/tools/test_get_lines.py index a7abbfb0666..10a55e713a0 100644 --- a/tests/tools/test_get_lines.py +++ b/tests/tools/test_get_lines.py @@ -4,7 +4,7 @@ import pytest -from cecli.tools import read_range +from cecli.tools import read_file class DummyIO: @@ -62,7 +62,7 @@ def coder_with_file(tmp_path): def test_pattern_with_zero_line_number_is_allowed(coder_with_file): coder, file_path = coder_with_file - result = read_range.Tool.execute( + result = read_file.Tool.execute( coder, read=[ { @@ -74,7 +74,7 @@ def test_pattern_with_zero_line_number_is_allowed(coder_with_file): ], ) - # read_range now returns a new formatted context message + # read_file now returns a new formatted context message assert "Retrieved context for 1 operation(s)" in result coder.io.tool_error.assert_not_called() @@ -82,7 +82,7 @@ def test_pattern_with_zero_line_number_is_allowed(coder_with_file): def test_empty_pattern_uses_line_number(coder_with_file): coder, file_path = coder_with_file - result = read_range.Tool.execute( + result = read_file.Tool.execute( coder, read=[ { @@ -94,7 +94,7 @@ def test_empty_pattern_uses_line_number(coder_with_file): ], ) - # read_range now returns a static success message + # read_file now returns a static success message assert "Retrieved context for 1 operation(s)" in result coder.io.tool_error.assert_not_called() @@ -104,7 +104,7 @@ def test_conflicting_pattern_and_line_number_raise(coder_with_file): # Test that missing start_text raises an error # Test that missing range_start raises an error - result = read_range.Tool.execute( + result = read_file.Tool.execute( coder, read=[ { @@ -139,7 +139,7 @@ def test_multiline_pattern_search(coder_with_file): coder, file_path = coder_with_file # file_path contains "alpha\nbeta\ngamma\n" - result = read_range.Tool.execute( + result = read_file.Tool.execute( coder, read=[ { @@ -166,7 +166,7 @@ def test_empty_file_includes_edit_hint(tmp_path): conv.get_files.return_value.clear_ranges = Mock() conv.get_files.return_value.push_range = Mock() conv.get_chunks.return_value.add_file_context_messages = Mock() - result = read_range.Tool.execute( + result = read_file.Tool.execute( coder, read=[ { @@ -178,6 +178,6 @@ def test_empty_file_includes_edit_hint(tmp_path): ) assert "pubspec.yaml is empty" in result - assert "EditText" in result - assert "readrange again" in result.lower() + assert "EditFile" in result + assert "readfile again" in result.lower() coder.io.tool_error.assert_not_called() diff --git a/tests/tools/test_grep.py b/tests/tools/test_grep.py index 82bfb3cb3cd..0228e98f042 100644 --- a/tests/tools/test_grep.py +++ b/tests/tools/test_grep.py @@ -51,6 +51,13 @@ def test_dash_prefixed_pattern_is_searched_literally(search_term, tmp_path, monk ], ) - assert "Matches for" in result - assert search_term in result + import json + + data = json.loads(result) + assert "operations" in data + assert len(data["operations"]) == 1 + op = data["operations"][0] + assert op["pattern"] == search_term + assert op["error"] is None + assert "total_files" in op coder.io.tool_error.assert_not_called() diff --git a/tests/tools/test_insert_block.py b/tests/tools/test_insert_block.py index 55598295b1b..ffca6f8e77b 100644 --- a/tests/tools/test_insert_block.py +++ b/tests/tools/test_insert_block.py @@ -5,7 +5,7 @@ import pytest from cecli.helpers.hashline import hashline -from cecli.tools import edit_text +from cecli.tools import edit_file class DummyIO: @@ -85,7 +85,7 @@ def test_position_top_succeeds_with_no_patterns(coder_with_file): hash_fragment = line1_hashline.split("::", 1)[0] # Everything before "::" start_line = hash_fragment # Just the hash fragment, no brackets - result = edit_text.Tool.execute( + result = edit_file.Tool.execute( coder, edits=[ { @@ -97,7 +97,7 @@ def test_position_top_succeeds_with_no_patterns(coder_with_file): ], ) - assert result.startswith("Successfully executed EditText.") + assert result.startswith("Successfully executed EditFile.") lines = file_path.read_text().splitlines() # Inserted line replaces first line (inclusive bounds) assert lines[1] == "second line" # Original second line shifts up @@ -109,7 +109,7 @@ def test_mutually_exclusive_parameters_raise(coder_with_file): coder, file_path = coder_with_file # Test with invalid hashline format (missing pipe) - result = edit_text.Tool.execute( + result = edit_file.Tool.execute( coder, edits=[ { @@ -121,8 +121,8 @@ def test_mutually_exclusive_parameters_raise(coder_with_file): ], ) - assert result.startswith("Error in EditText:") - assert "Invalid Edit - Update content ID bounds" in result + assert result.startswith("Error in EditFile:") + assert "Invalid Edit - Review content ID bounds" in result assert file_path.read_text().startswith("first line") coder.io.tool_error.assert_called() @@ -139,7 +139,7 @@ def test_trailing_newline_preservation(coder_with_file): hash_fragment = line1_hashline.split("::", 1)[0] # Everything before "::" start_line = hash_fragment # Just the hash fragment, no brackets - edit_text.Tool.execute( + edit_file.Tool.execute( coder, edits=[ { @@ -176,7 +176,7 @@ def test_no_trailing_newline_preservation(coder_with_file): # Extract hash fragment from {hash}::content format hash_fragment = line1_hashline.split("::", 1)[0] # Everything before "::" start_line = hash_fragment # Just the hash fragment, no brackets - edit_text.Tool.execute( + edit_file.Tool.execute( coder, edits=[ { @@ -209,7 +209,7 @@ def test_line_number_beyond_file_length_appends(coder_with_file): # Extract hash fragment from {hash}::content format hash_fragment = line2_hashline.split("::", 1)[0] # Everything before "::" start_line = hash_fragment # Just the hash fragment, no brackets - result = edit_text.Tool.execute( + result = edit_file.Tool.execute( coder, edits=[ { @@ -221,7 +221,7 @@ def test_line_number_beyond_file_length_appends(coder_with_file): ], ) - assert result.startswith("Successfully executed EditText.") + assert result.startswith("Successfully executed EditFile.") content = file_path.read_text() assert content == "first line\nappended line\n" coder.io.tool_error.assert_not_called() @@ -242,7 +242,7 @@ def test_line_number_beyond_file_length_appends_no_trailing_newline(coder_with_f hash_fragment = line2_hashline.split("::", 1)[0] # Everything before "::" start_line = hash_fragment # Just the hash fragment, no brackets - edit_text.Tool.execute( + edit_file.Tool.execute( coder, edits=[ { diff --git a/tests/tools/test_read_range_execute.py b/tests/tools/test_read_range_execute.py index a985a6452b9..7006f0b0185 100644 --- a/tests/tools/test_read_range_execute.py +++ b/tests/tools/test_read_range_execute.py @@ -1,5 +1,5 @@ """ -Tests for the execute method of read_range.py. +Tests for the execute method of read_file.py. Focuses on the parsing logic for line numbers, special markers (@000, 000@), and text strings. Tests cover all combinations of these marker types. @@ -90,7 +90,7 @@ def create_test_file(content): # ============================================================================= -class TestReadRangeExecute: +class TestReadFileExecute: """Tests for Tool.execute() parsing logic.""" # Class-level patches that apply to all tests @@ -118,13 +118,13 @@ def _setup(self, mock_coder, mock_file_context, mock_chunks, mock_manager, file_ self.patches.append(cs_patch) # Patch strip_hashline to be identity - sh_patch = patch("cecli.tools.read_range.strip_hashline", side_effect=lambda x: x) + sh_patch = patch("cecli.tools.read_file.strip_hashline", side_effect=lambda x: x) sh_patch.start() self.patches.append(sh_patch) # Patch hashline_formatted to return (text, json) hl_patch = patch( - "cecli.tools.read_range.hashline_formatted", + "cecli.tools.read_file.hashline_formatted", side_effect=lambda text, file_name, partial, expanded, start_line=1: (text, "{}"), ) hl_patch.start() @@ -132,7 +132,7 @@ def _setup(self, mock_coder, mock_file_context, mock_chunks, mock_manager, file_ # Patch resolve_paths rp_patch = patch( - "cecli.tools.read_range.resolve_paths", + "cecli.tools.read_file.resolve_paths", return_value=(self.test_file, _safe_relpath(self.test_file)), ) rp_patch.start() @@ -140,14 +140,14 @@ def _setup(self, mock_coder, mock_file_context, mock_chunks, mock_manager, file_ # Patch is_provided ip_patch = patch( - "cecli.tools.read_range.is_provided", + "cecli.tools.read_file.is_provided", side_effect=lambda v, **kw: v is not None and v != "", ) ip_patch.start() self.patches.append(ip_patch) # Reset class-level state on Tool - from cecli.tools.read_range import Tool + from cecli.tools.read_file import Tool self.Tool = Tool Tool._last_invocation = {} @@ -410,12 +410,12 @@ def test_file_not_found(self, mock_coder, mock_file_context, mock_chunks, mock_m mock_coder.abs_root_path.return_value = abs_path rp_patch = patch( - "cecli.tools.read_range.resolve_paths", return_value=(abs_path, "nonexistent/path.py") + "cecli.tools.read_file.resolve_paths", return_value=(abs_path, "nonexistent/path.py") ) rp_patch.start() self.patches.append(rp_patch) - from cecli.tools.read_range import Tool + from cecli.tools.read_file import Tool show = [{"file_path": "nonexistent/path.py", "range_start": "1", "range_end": "10"}] result = Tool.execute(mock_coder, show) @@ -423,7 +423,7 @@ def test_file_not_found(self, mock_coder, mock_file_context, mock_chunks, mock_m def test_missing_parameters(self, mock_coder, mock_file_context, mock_chunks, mock_manager): """Test with missing range_start and range_end (empty strings).""" - from cecli.tools.read_range import Tool + from cecli.tools.read_file import Tool show = [{"file_path": "some_file.py", "range_start": "", "range_end": ""}] result = Tool.execute(mock_coder, show) @@ -443,20 +443,20 @@ def resolve_side_effect(coder, file_path): return (test_file1, "file1.py") return (test_file2, "file2.py") - rp_patch = patch("cecli.tools.read_range.resolve_paths", side_effect=resolve_side_effect) + rp_patch = patch("cecli.tools.read_file.resolve_paths", side_effect=resolve_side_effect) rp_patch.start() - sh_patch = patch("cecli.tools.read_range.strip_hashline", side_effect=lambda x: x) + sh_patch = patch("cecli.tools.read_file.strip_hashline", side_effect=lambda x: x) sh_patch.start() hl_patch = patch( - "cecli.tools.read_range.hashline_formatted", + "cecli.tools.read_file.hashline_formatted", side_effect=lambda text, file_name, partial, expanded, start_line=1: (text, "{}"), ) hl_patch.start() ip_patch = patch( - "cecli.tools.read_range.is_provided", + "cecli.tools.read_file.is_provided", side_effect=lambda v, **kw: v is not None and v != "", ) ip_patch.start() @@ -472,7 +472,7 @@ def resolve_side_effect(coder, file_path): mock_coder.io.read_text.side_effect = lambda path: content_map.get(path, "") try: - from cecli.tools.read_range import Tool + from cecli.tools.read_file import Tool Tool._last_invocation = {} Tool._last_read_turn = {} diff --git a/tests/tools/test_registry.py b/tests/tools/test_registry.py index 5cbddf7d529..a83856bbc5f 100644 --- a/tests/tools/test_registry.py +++ b/tests/tools/test_registry.py @@ -28,7 +28,7 @@ def test_registry_initialization(self): assert len(tools) > 0, "Registry should have tools after initialization" # Check that essential tools are registered - essential_tools = {"resourcemanager", "edittext", "yield"} + essential_tools = {"resourcemanager", "editfile", "yield"} for tool in essential_tools: assert tool in tools, f"Essential tool {tool} should be registered" @@ -53,18 +53,18 @@ def test_build_registry_empty_config(self): # Essential tools should always be included assert "resourcemanager" in registry, "Essential tool should be included" - assert "edittext" in registry, "Essential tool should be included" + assert "editfile" in registry, "Essential tool should be included" assert "yield" in registry, "Essential tool should be included" def test_build_registry_with_includelist(self): """Test filtering with tools_includelist""" - config = {"tools_includelist": ["resourcemanager", "edittext"]} + config = {"tools_includelist": ["resourcemanager", "editfile"]} registry = ToolRegistry.build_registry(config) # Should only include tools from includelist, plus essential tools assert len(registry) == 3, "Should include 2 from list + 1 essential" assert "resourcemanager" in registry - assert "edittext" in registry + assert "editfile" in registry assert "yield" in registry # Essential assert "command" not in registry, "Should not include tools not in includelist" @@ -80,19 +80,19 @@ def test_build_registry_with_excludelist(self): def test_build_registry_exclude_essential(self): """Test that essential tools cannot be excluded""" - config = {"tools_excludelist": ["resourcemanager", "edittext", "finished", "command"]} + config = {"tools_excludelist": ["resourcemanager", "editfile", "finished", "command"]} registry = ToolRegistry.build_registry(config) # Essential tools should still be included despite excludelist assert "resourcemanager" in registry, "Essential tool cannot be excluded" - assert "edittext" in registry, "Essential tool cannot be excluded" + assert "editfile" in registry, "Essential tool cannot be excluded" assert "yield" in registry, "Essential tool cannot be excluded" assert "command" not in registry, "Non-essential tool should be excluded" def test_build_registry_combined_filters(self): """Test combined filtering with includelist and excludelist""" config = { - "tools_includelist": ["resourcemanager", "edittext", "command"], + "tools_includelist": ["resourcemanager", "editfile", "command"], "tools_excludelist": ["commandinteractive"], } registry = ToolRegistry.build_registry(config) @@ -100,36 +100,36 @@ def test_build_registry_combined_filters(self): # Should respect all filters assert len(registry) == 4, "Should include exactly 4 tools (3 from list + yield)" assert "resourcemanager" in registry - assert "edittext" in registry + assert "editfile" in registry assert "yield" in registry assert "command" in registry assert "commandinteractive" not in registry def test_get_filtered_tools(self): """Test get_filtered_tools method""" - config = {"tools_includelist": ["resourcemanager", "edittext"]} + config = {"tools_includelist": ["resourcemanager", "editfile"]} ToolRegistry.build_registry(config) tool_names = ToolRegistry.get_registered_tools() # Should return list of tool names assert isinstance(tool_names, list) - # Should include resourcemanager, edittext, and finished (essential) + # Should include resourcemanager, editfile, and finished (essential) assert len(tool_names) == 3 assert "resourcemanager" in tool_names - assert "edittext" in tool_names + assert "editfile" in tool_names assert "yield" in tool_names # Essential tool always included def test_legacy_config_names(self): """Test backward compatibility with legacy config names (whitelist/blacklist)""" config = { - "tools_whitelist": ["resourcemanager", "edittext"], + "tools_whitelist": ["resourcemanager", "editfile"], "tools_blacklist": ["command"], } registry = ToolRegistry.build_registry(config) # Should work with legacy names assert "resourcemanager" in registry - assert "edittext" in registry + assert "editfile" in registry assert "command" not in registry def test_config_precedence(self): @@ -152,7 +152,7 @@ def test_config_precedence(self): def test_registry_consistency(self): """Test that registry methods return consistent results""" - config = {"tools_includelist": ["resourcemanager", "edittext"]} + config = {"tools_includelist": ["resourcemanager", "editfile"]} # build_registry should return consistent results registry = ToolRegistry.build_registry(config)