From 08b138c29003f7e4efe76991d13954ddc1373362 Mon Sep 17 00:00:00 2001 From: Hemdan47 Date: Mon, 22 Jun 2026 04:18:04 +0300 Subject: [PATCH 1/8] fix: (bash-tool) classify mutating git subcommands as modify --- src/agent/tools/bash_tool/bash.py | 30 ++++++++++++++------ tests/unit/agent/tools/test_bash_tool.py | 36 ++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 8 deletions(-) diff --git a/src/agent/tools/bash_tool/bash.py b/src/agent/tools/bash_tool/bash.py index 1065cfc..17628a0 100644 --- a/src/agent/tools/bash_tool/bash.py +++ b/src/agent/tools/bash_tool/bash.py @@ -134,22 +134,36 @@ def _classify_single_command(self, cmd: str) -> str: cmd_parts = cmd.strip().split() if not cmd_parts: return "unknown" - + + # Heuristic: output redirection modifies files regardless of base command. + # Checked first so e.g. `git status > out.txt` is never treated as safe. + if self._has_write_redirection(cmd): + return "modify" + base_cmd = cmd_parts[0] - + # Special handling for git commands if base_cmd == 'git' and len(cmd_parts) > 1: git_subcmd = cmd_parts[1] + remainder = cmd_parts[2:] + + # branch/tag/remote/config are read-only ONLY when used bare; + # `git branch` lists branches, but `git branch feat`/`-D`/`-m` + # create/delete/rename, so any argument is treated as a modify. + if git_subcmd in {'branch', 'tag', 'remote', 'config'}: + return "safe" if not remainder else "modify" + + # `git stash` is safe only for its list/show read-only sub-actions. + if git_subcmd == 'stash': + if remainder and remainder[0] in {'list', 'show'}: + return "safe" + return "modify" # stash push/pop/drop/clear/... + full_git_cmd = f"git {git_subcmd}" if full_git_cmd in self.safe_commands: return "safe" - else: - return "modify" # git add, commit, push, etc. + return "modify" # git add, commit, push, etc. - # Heuristic: output redirection modifies files regardless of base command. - if self._has_write_redirection(cmd): - return "modify" - # Check command classification if base_cmd in self.dangerous_commands: return "dangerous" diff --git a/tests/unit/agent/tools/test_bash_tool.py b/tests/unit/agent/tools/test_bash_tool.py index 3df029e..83405d1 100644 --- a/tests/unit/agent/tools/test_bash_tool.py +++ b/tests/unit/agent/tools/test_bash_tool.py @@ -45,6 +45,42 @@ def test_git_modify_commands(self): assert self.tool._classify_single_command("git commit -m 'msg'") == "modify" assert self.tool._classify_single_command("git push") == "modify" + def test_git_bare_only_subcommands_modify_with_args(self): + # branch/tag/remote/config are read-only only when used bare + assert self.tool._classify_single_command("git branch feature") == "modify" + assert self.tool._classify_single_command("git branch -D main") == "modify" + assert self.tool._classify_single_command("git branch -m renamed") == "modify" + assert self.tool._classify_single_command("git tag v1.0") == "modify" + assert self.tool._classify_single_command("git tag -d v1.0") == "modify" + assert self.tool._classify_single_command("git remote add upstream url") == "modify" + assert self.tool._classify_single_command("git remote set-url origin url") == "modify" + assert self.tool._classify_single_command('git config user.name "X"') == "modify" + assert self.tool._classify_single_command("git config --global core.editor vim") == "modify" + + def test_git_bare_only_subcommands_safe_when_bare(self): + assert self.tool._classify_single_command("git branch") == "safe" + assert self.tool._classify_single_command("git tag") == "safe" + assert self.tool._classify_single_command("git remote") == "safe" + assert self.tool._classify_single_command("git config") == "safe" + + def test_git_stash_readonly(self): + assert self.tool._classify_single_command("git stash list") == "safe" + assert self.tool._classify_single_command("git stash show") == "safe" + assert self.tool._classify_single_command("git stash") == "modify" + assert self.tool._classify_single_command("git stash drop") == "modify" + assert self.tool._classify_single_command("git stash pop") == "modify" + + def test_git_safe_commands_accept_args(self): + assert self.tool._classify_single_command("git status -s") == "safe" + assert self.tool._classify_single_command("git log --oneline") == "safe" + assert self.tool._classify_single_command("git diff HEAD~1") == "safe" + assert self.tool._classify_single_command("git show HEAD") == "safe" + + def test_git_redirection_is_modify(self): + assert self.tool._classify_single_command("git status > out.txt") == "modify" + assert self.tool._classify_single_command("git log >> out.txt") == "modify" + assert self.tool._classify_single_command("git branch >> out.txt") == "modify" + class TestBashToolCommandChain: """Test command chain parsing.""" From 32a6905b4db435301ca5375c75fa9c047e6b713b Mon Sep 17 00:00:00 2001 From: Hemdan47 Date: Mon, 22 Jun 2026 05:28:45 +0300 Subject: [PATCH 2/8] refactor: Convert glob/grep/list tools to BaseTool classes with configurable working_directory Bug: These tools defaulted to os.getcwd() at call time, causing them to search the agent's repo instead of the target project when the process cwd was set to the agent directory. BashTool didn't have this issue because it already used the BaseTool class pattern with injected working_directory. Changes: - Convert glob_tool, grep_tool, list_tool from @tool functions to BaseTool subclasses - Add create_glob_tool(), create_grep_tool(), create_list_tool() factories - Store working_directory as instance attribute; use as default when path omitted - Update analyzer.py to construct tools with working_directory=project_path --- src/agent/graph/subgraphs/analyzer.py | 12 +- src/agent/tools/__init__.py | 12 +- src/agent/tools/glob_tool/__init__.py | 4 +- src/agent/tools/glob_tool/glob.py | 230 +++++++++------ src/agent/tools/grep_tool/__init__.py | 4 +- src/agent/tools/grep_tool/grep.py | 268 +++++++++++------- src/agent/tools/list_tool/__init__.py | 4 +- src/agent/tools/list_tool/list.py | 238 ++++++++++------ tests/unit/agent/tools/test_glob_tool.py | 44 ++- tests/unit/agent/tools/test_grep_tool.py | 46 ++- tests/unit/agent/tools/test_list_tool.py | 45 ++- .../tools/test_streaming_instrumentation.py | 12 +- 12 files changed, 623 insertions(+), 296 deletions(-) diff --git a/src/agent/graph/subgraphs/analyzer.py b/src/agent/graph/subgraphs/analyzer.py index 1597e0d..6df321d 100644 --- a/src/agent/graph/subgraphs/analyzer.py +++ b/src/agent/graph/subgraphs/analyzer.py @@ -20,9 +20,9 @@ from agent.exceptions import PermissionRequiredException from agent.tools import ( create_bash_tool, - glob_tool, - grep_tool, - list_tool, + create_glob_tool, + create_grep_tool, + create_list_tool, read_tool, ) @@ -78,9 +78,9 @@ def build_analyzer_agent(project_path: str, llm=None, approved_commands: set[str working_directory=project_path, approved_commands=approved_commands or set(), ), - glob_tool, - grep_tool, - list_tool, + create_glob_tool(working_directory=project_path), + create_grep_tool(working_directory=project_path), + create_list_tool(working_directory=project_path), read_tool, ] diff --git a/src/agent/tools/__init__.py b/src/agent/tools/__init__.py index 94a09b4..df8a6c1 100644 --- a/src/agent/tools/__init__.py +++ b/src/agent/tools/__init__.py @@ -4,9 +4,9 @@ """ from agent.tools.bash_tool.bash import create_bash_tool -from agent.tools.glob_tool.glob import glob_tool -from agent.tools.grep_tool.grep import grep_tool -from agent.tools.list_tool.list import list_tool +from agent.tools.glob_tool.glob import create_glob_tool +from agent.tools.grep_tool.grep import create_grep_tool +from agent.tools.list_tool.list import create_list_tool from agent.tools.read_tool.read import read as read_tool from agent.tools.write_tool.write import write as write_tool from agent.tools.todo_tool.todo import todowrite, todoread @@ -14,9 +14,9 @@ __all__ = [ "create_bash_tool", - "glob_tool", - "grep_tool", - "list_tool", + "create_glob_tool", + "create_grep_tool", + "create_list_tool", "read_tool", "write_tool", "todowrite", diff --git a/src/agent/tools/glob_tool/__init__.py b/src/agent/tools/glob_tool/__init__.py index 7e8b940..ac6d31b 100644 --- a/src/agent/tools/glob_tool/__init__.py +++ b/src/agent/tools/glob_tool/__init__.py @@ -1 +1,3 @@ -from .glob import glob_tool +from .glob import GlobTool, create_glob_tool + +__all__ = ["GlobTool", "create_glob_tool"] diff --git a/src/agent/tools/glob_tool/glob.py b/src/agent/tools/glob_tool/glob.py index 4398f03..5c0b9f3 100644 --- a/src/agent/tools/glob_tool/glob.py +++ b/src/agent/tools/glob_tool/glob.py @@ -1,95 +1,157 @@ -from langchain_core.tools import tool import os from pathlib import Path import glob +from typing import Optional, Type + +from langchain_core.tools import BaseTool +from langchain_core.callbacks.manager import CallbackManagerForToolRun +from pydantic import BaseModel, Field from agent.streaming.tool_run import tool_run -@tool("glob") -def glob_tool(pattern: str, path: str = None, limit: int = 100): - """ - Search files matching a glob pattern in a directory. - - Args: - pattern (str): Glob pattern (e.g. "*.py"). - path (str, optional): Directory to search in. Defaults to cwd. - limit (int): Maximum number of results. +DESCRIPTION = """Search files matching a glob pattern in a directory. - - Returns: - dict: { - "title": str, # relative path of search directory - "metadata": { "count": int, "truncated": bool }, - "output": str # human readable file list - } - """ - # get the absolute path of the directory. - search = Path(path or os.getcwd()).resolve() - - with tool_run( - "glob", - f'Finding files matching "{pattern}"', - {"pattern": pattern, "path": str(search)}, - ) as ctx: - # Use os.walk for recursive ** patterns (more reliable on Windows) - matches = [] - - if pattern.startswith("**/"): - # Pattern like "**/package.json" - search recursively for filename - filename = pattern[3:] # Remove "**/ - for root, dirs, files in os.walk(str(search)): - if filename in files: - matches.append(os.path.join(root, filename)) - elif "**" in pattern: - # Pattern contains ** somewhere - use glob with recursive - if pattern.startswith("**"): - search_pattern = os.path.join(str(search), pattern) +Args: + pattern (str): Glob pattern (e.g. "*.py"). + path (str, optional): Directory to search in. Defaults to the tool's working directory. + limit (int): Maximum number of results. + +Returns: + dict: { + "title": str, # relative path of search directory + "metadata": { "count": int, "truncated": bool }, + "output": str # human readable file list + } +""" + + +class GlobToolInput(BaseModel): + """Input schema for the glob tool.""" + pattern: str = Field(description="Glob pattern (e.g. '*.py')") + path: Optional[str] = Field( + default=None, + description="Directory to search in. If not provided, searches the working directory.", + ) + limit: int = Field(default=100, description="Maximum number of results") + + +class GlobTool(BaseTool): + """LangChain tool for searching files with glob patterns.""" + + name: str = "glob" + description: str = DESCRIPTION + args_schema: Type[BaseModel] = GlobToolInput + return_direct: bool = False + + def __init__(self, working_directory: Optional[str] = None, **kwargs): + super().__init__(**kwargs) + # Use object.__setattr__ to bypass Pydantic's field validation + object.__setattr__(self, 'working_dir', os.path.abspath(working_directory or os.getcwd())) + + def _run( + self, + pattern: str, + path: Optional[str] = None, + limit: int = 100, + run_manager: Optional[CallbackManagerForToolRun] = None, + ) -> dict: + """Execute the glob search.""" + # Use provided path or fall back to working_dir + search = Path(path or self.working_dir).resolve() + + with tool_run( + "glob", + f'Finding files matching "{pattern}"', + {"pattern": pattern, "path": str(search)}, + ) as ctx: + # Use os.walk for recursive ** patterns (more reliable on Windows) + matches = [] + + if pattern.startswith("**/"): + # Pattern like "**/package.json" - search recursively for filename + filename = pattern[3:] # Remove "**/ + for root, dirs, files in os.walk(str(search)): + if filename in files: + matches.append(os.path.join(root, filename)) + elif "**" in pattern: + # Pattern contains ** somewhere - use glob with recursive + if pattern.startswith("**"): + search_pattern = os.path.join(str(search), pattern) + else: + search_pattern = str(search / pattern) + matches = glob.glob(search_pattern, recursive=True) else: + # Simple pattern without ** - use glob normally search_pattern = str(search / pattern) - matches = glob.glob(search_pattern, recursive=True) - else: - # Simple pattern without ** - use glob normally - search_pattern = str(search / pattern) - matches = glob.glob(search_pattern, recursive=False) - - # collect matches and their modification time - files = [] - for file in matches: - stat = os.stat(file) - mtime = stat.st_mtime - files.append({"path": str(Path(file).resolve()), "mtime": mtime}) - - files.sort(key=lambda f: f["mtime"], reverse=True) - - truncated = len(files) > limit - if truncated: - files = files[:limit] - - output_lines = [] - if not files: - output_lines.append("No files found") - else: - output_lines.extend(f["path"] for f in files) + matches = glob.glob(search_pattern, recursive=False) + + # collect matches and their modification time + files = [] + for file in matches: + stat = os.stat(file) + mtime = stat.st_mtime + files.append({"path": str(Path(file).resolve()), "mtime": mtime}) + + files.sort(key=lambda f: f["mtime"], reverse=True) + + truncated = len(files) > limit if truncated: - output_lines.append("") - output_lines.append("(Results are truncated. Consider using a more specific path or pattern.)") - - # Get relative path, but handle cases where search is outside cwd - try: - title = str(search.relative_to(Path.cwd())) - except ValueError: - # If search path is not relative to cwd, just use the directory name - title = search.name - - ctx["result_label"] = f"Found {len(files)} files" - ctx["result_data"] = {"count": len(files), "truncated": truncated} - - return { - "title": title, - "metadata": { - "count": len(files), - "truncated": truncated, - }, - "output": "\n".join(output_lines), - } + files = files[:limit] + + output_lines = [] + if not files: + output_lines.append("No files found") + else: + output_lines.extend(f["path"] for f in files) + if truncated: + output_lines.append("") + output_lines.append("(Results are truncated. Consider using a more specific path or pattern.)") + + # Get relative path, but handle cases where search is outside cwd + try: + title = str(search.relative_to(Path.cwd())) + except ValueError: + # If search path is not relative to cwd, just use the directory name + title = search.name + + ctx["result_label"] = f"Found {len(files)} files" + ctx["result_data"] = {"count": len(files), "truncated": truncated} + + return { + "title": title, + "metadata": { + "count": len(files), + "truncated": truncated, + }, + "output": "\n".join(output_lines), + } + + async def _arun( + self, + pattern: str, + path: Optional[str] = None, + limit: int = 100, + run_manager: Optional[CallbackManagerForToolRun] = None, + ) -> dict: + """Async version - just calls the sync version.""" + return self._run(pattern, path, limit, run_manager) + + +def create_glob_tool(working_directory: Optional[str] = None) -> GlobTool: + """ + Factory function to create a GlobTool instance for LangChain. + + Args: + working_directory: Directory to search in by default (default: current directory) + + Returns: + Configured GlobTool instance ready for use with LangChain + + Example: + >>> from agent.tools import create_glob_tool + >>> glob_tool = create_glob_tool(working_directory="./project") + >>> # Use with LangChain agent + >>> tools = [glob_tool] + """ + return GlobTool(working_directory=working_directory) diff --git a/src/agent/tools/grep_tool/__init__.py b/src/agent/tools/grep_tool/__init__.py index 6b23098..0bc6a80 100644 --- a/src/agent/tools/grep_tool/__init__.py +++ b/src/agent/tools/grep_tool/__init__.py @@ -1 +1,3 @@ -from .grep import grep_tool +from .grep import GrepTool, create_grep_tool + +__all__ = ["GrepTool", "create_grep_tool"] diff --git a/src/agent/tools/grep_tool/grep.py b/src/agent/tools/grep_tool/grep.py index 28fdf23..25d4560 100644 --- a/src/agent/tools/grep_tool/grep.py +++ b/src/agent/tools/grep_tool/grep.py @@ -2,7 +2,11 @@ import re import fnmatch from pathlib import Path -from langchain_core.tools import tool +from typing import Optional, Type + +from langchain_core.tools import BaseTool +from langchain_core.callbacks.manager import CallbackManagerForToolRun +from pydantic import BaseModel, Field from agent.streaming.tool_run import tool_run @@ -28,109 +32,175 @@ def _is_binary(filepath: str) -> bool: return Path(filepath).suffix.lower() in _BINARY_EXT -@tool("grep") -def grep_tool(pattern: str, path: str = None, include: str = None, limit: int = 100): - """ - Search for a regex pattern in files. - - Args: - pattern (str): Regex pattern to search for. - path (str, optional): Directory to search in. Defaults to cwd. - include (str, optional): File glob pattern to include (e.g. "*.py") - limit (int): Maximum number of matches to return. - - Returns: - dict: { - "title": str, - "metadata": { "count": int, "truncated": bool }, - "output": str - } - """ - search_path = Path(path or os.getcwd()).resolve() - with tool_run( - "grep", - f'Searching for "{pattern}"', - {"pattern": pattern, "path": str(search_path), "include": include or ""}, - ) as ctx: - if not pattern: - raise ValueError("pattern is required") - - regex = re.compile(pattern) - - matches = [] - found_enough = False - - for root, dirs, files in os.walk(str(search_path)): - # Skip common non-source directories - dirs[:] = [d for d in dirs if d not in _SKIP_DIRS] - - for filename in files: - # Apply include filter - if include and not fnmatch.fnmatch(filename, include): - continue - - filepath = os.path.join(root, filename) - - # Skip binary files - if _is_binary(filepath): - continue - - try: - with open(filepath, "r", encoding="utf-8", errors="ignore") as f: - for line_num, line in enumerate(f, 1): - if regex.search(line): - matches.append({ - "path": str(Path(filepath).resolve()), - "lineNum": line_num, - "lineText": line.rstrip("\n\r"), - }) - - if len(matches) > limit: - found_enough = True - break - except (OSError, PermissionError): - continue - +DESCRIPTION = """Search for a regex pattern in files. + +Args: + pattern (str): Regex pattern to search for. + path (str, optional): Directory to search in. Defaults to the tool's working directory. + include (str, optional): File glob pattern to include (e.g. "*.py") + limit (int): Maximum number of matches to return. + +Returns: + dict: { + "title": str, + "metadata": { "count": int, "truncated": bool }, + "output": str + } +""" + + +class GrepToolInput(BaseModel): + """Input schema for the grep tool.""" + pattern: str = Field(description="Regex pattern to search for") + path: Optional[str] = Field( + default=None, + description="Directory to search in. If not provided, searches the working directory.", + ) + include: Optional[str] = Field( + default=None, + description="File glob pattern to include (e.g. '*.py')" + ) + limit: int = Field(default=100, description="Maximum number of matches to return") + + +class GrepTool(BaseTool): + """LangChain tool for searching text with regex patterns.""" + + name: str = "grep" + description: str = DESCRIPTION + args_schema: Type[BaseModel] = GrepToolInput + return_direct: bool = False + + def __init__(self, working_directory: Optional[str] = None, **kwargs): + super().__init__(**kwargs) + # Use object.__setattr__ to bypass Pydantic's field validation + object.__setattr__(self, 'working_dir', os.path.abspath(working_directory or os.getcwd())) + + def _run( + self, + pattern: str, + path: Optional[str] = None, + include: Optional[str] = None, + limit: int = 100, + run_manager: Optional[CallbackManagerForToolRun] = None, + ) -> dict: + """Execute the grep search.""" + # Use provided path or fall back to working_dir + search_path = Path(path or self.working_dir).resolve() + with tool_run( + "grep", + f'Searching for "{pattern}"', + {"pattern": pattern, "path": str(search_path), "include": include or ""}, + ) as ctx: + if not pattern: + raise ValueError("pattern is required") + + regex = re.compile(pattern) + + matches = [] + found_enough = False + + for root, dirs, files in os.walk(str(search_path)): + # Skip common non-source directories + dirs[:] = [d for d in dirs if d not in _SKIP_DIRS] + + for filename in files: + # Apply include filter + if include and not fnmatch.fnmatch(filename, include): + continue + + filepath = os.path.join(root, filename) + + # Skip binary files + if _is_binary(filepath): + continue + + try: + with open(filepath, "r", encoding="utf-8", errors="ignore") as f: + for line_num, line in enumerate(f, 1): + if regex.search(line): + matches.append({ + "path": str(Path(filepath).resolve()), + "lineNum": line_num, + "lineText": line.rstrip("\n\r"), + }) + + if len(matches) > limit: + found_enough = True + break + except (OSError, PermissionError): + continue + + if found_enough: + break if found_enough: break - if found_enough: - break - # Handle truncation - truncated = len(matches) > limit - if truncated: - matches = matches[:limit] + # Handle truncation + truncated = len(matches) > limit + if truncated: + matches = matches[:limit] + + ctx["result_label"] = f'Found {len(matches)} matches for "{pattern}"' + ctx["result_data"] = {"count": len(matches), "truncated": truncated} + + if not matches: + return { + "title": pattern, + "metadata": {"count": 0, "truncated": False}, + "output": "No files found", + } + + # Format output grouped by file + output_lines = [f"Found {len(matches)} matches"] + current_file = "" + for m in matches: + if current_file != m["path"]: + if current_file: + output_lines.append("") + current_file = m["path"] + output_lines.append(f"{m['path']}:") + output_lines.append(f" Line {m['lineNum']}: {m['lineText']}") + + if truncated: + output_lines.append("") + output_lines.append("(Results are truncated. Consider using a more specific path or pattern.)") - ctx["result_label"] = f'Found {len(matches)} matches for "{pattern}"' - ctx["result_data"] = {"count": len(matches), "truncated": truncated} - - if not matches: return { "title": pattern, - "metadata": {"count": 0, "truncated": False}, - "output": "No files found", + "metadata": { + "count": len(matches), + "truncated": truncated, + }, + "output": "\n".join(output_lines), } - # Format output grouped by file - output_lines = [f"Found {len(matches)} matches"] - current_file = "" - for m in matches: - if current_file != m["path"]: - if current_file: - output_lines.append("") - current_file = m["path"] - output_lines.append(f"{m['path']}:") - output_lines.append(f" Line {m['lineNum']}: {m['lineText']}") - - if truncated: - output_lines.append("") - output_lines.append("(Results are truncated. Consider using a more specific path or pattern.)") - - return { - "title": pattern, - "metadata": { - "count": len(matches), - "truncated": truncated, - }, - "output": "\n".join(output_lines), - } + async def _arun( + self, + pattern: str, + path: Optional[str] = None, + include: Optional[str] = None, + limit: int = 100, + run_manager: Optional[CallbackManagerForToolRun] = None, + ) -> dict: + """Async version - just calls the sync version.""" + return self._run(pattern, path, include, limit, run_manager) + + +def create_grep_tool(working_directory: Optional[str] = None) -> GrepTool: + """ + Factory function to create a GrepTool instance for LangChain. + + Args: + working_directory: Directory to search in by default (default: current directory) + + Returns: + Configured GrepTool instance ready for use with LangChain + + Example: + >>> from agent.tools import create_grep_tool + >>> grep_tool = create_grep_tool(working_directory="./project") + >>> # Use with LangChain agent + >>> tools = [grep_tool] + """ + return GrepTool(working_directory=working_directory) diff --git a/src/agent/tools/list_tool/__init__.py b/src/agent/tools/list_tool/__init__.py index b20f34f..9288675 100644 --- a/src/agent/tools/list_tool/__init__.py +++ b/src/agent/tools/list_tool/__init__.py @@ -1 +1,3 @@ -from .list import list_tool +from .list import ListTool, create_list_tool + +__all__ = ["ListTool", "create_list_tool"] diff --git a/src/agent/tools/list_tool/list.py b/src/agent/tools/list_tool/list.py index 82306cf..6c5f692 100644 --- a/src/agent/tools/list_tool/list.py +++ b/src/agent/tools/list_tool/list.py @@ -1,8 +1,11 @@ import fnmatch import os from pathlib import Path -from typing import Dict, List, Optional -from langchain_core.tools import tool +from typing import Dict, List, Optional, Type + +from langchain_core.tools import BaseTool +from langchain_core.callbacks.manager import CallbackManagerForToolRun +from pydantic import BaseModel, Field from agent.streaming.tool_run import tool_run @@ -33,109 +36,172 @@ "env/", ] -@tool("list") -def list_tool(path: str = "", ignore: Optional[List[str]] = None, limit: int = 100): - """ - List files and directories in a tree-like structure, with ignore rules. - - Args: - path (str, optional): Directory to list files from. Defaults to cwd. - ignore (List[str], optional): Extra glob patterns to ignore. - limit (int): Maximum number of matches to return. - - - Returns: - dict: { - "title": str, # base directory name (relative to worktree) - "metadata": { - "count": int, # number of files listed - "truncated": bool # True if file count reached the limit - }, - "output": str # human-readable directory tree - } - """ - resolved = Path(path if path else os.getcwd()).resolve() - with tool_run("list", f"Listing {resolved.name}", {"path": str(resolved)}) as ctx: - ignore_patterns = IGNORE_PATTERNS + (ignore if ignore else []) - files = [] - - for root, dirs, filenames in os.walk(resolved): - dirs[:] = [d for d in dirs if not any(fnmatch.fnmatch(d, pat.strip("/")) for pat in ignore_patterns)] - for filename in filenames: - rel_path = str(Path(root).relative_to(resolved) / filename) - if any(fnmatch.fnmatch(rel_path, pat.strip("/")) for pat in ignore_patterns): - continue - files.append(rel_path) +DESCRIPTION = """List files and directories in a tree-like structure, with ignore rules. + +Args: + path (str, optional): Directory to list files from. Defaults to the tool's working directory. + ignore (List[str], optional): Extra glob patterns to ignore. + limit (int): Maximum number of matches to return. + +Returns: + dict: { + "title": str, # base directory name (relative to worktree) + "metadata": { + "count": int, # number of files listed + "truncated": bool # True if file count reached the limit + }, + "output": str # human-readable directory tree + } +""" + + +class ListToolInput(BaseModel): + """Input schema for the list tool.""" + path: Optional[str] = Field( + default=None, + description="Directory to list files from. If not provided, lists the working directory.", + ) + ignore: Optional[List[str]] = Field( + default=None, + description="Extra glob patterns to ignore" + ) + limit: int = Field(default=100, description="Maximum number of matches to return") + + +class ListTool(BaseTool): + """LangChain tool for listing directory contents in tree format.""" + + name: str = "list" + description: str = DESCRIPTION + args_schema: Type[BaseModel] = ListToolInput + return_direct: bool = False + + def __init__(self, working_directory: Optional[str] = None, **kwargs): + super().__init__(**kwargs) + # Use object.__setattr__ to bypass Pydantic's field validation + object.__setattr__(self, 'working_dir', os.path.abspath(working_directory or os.getcwd())) + + def _run( + self, + path: Optional[str] = None, + ignore: Optional[List[str]] = None, + limit: int = 100, + run_manager: Optional[CallbackManagerForToolRun] = None, + ) -> dict: + """Execute the directory listing.""" + # Use provided path or fall back to working_dir + resolved = Path(path if path else self.working_dir).resolve() + with tool_run("list", f"Listing {resolved.name}", {"path": str(resolved)}) as ctx: + ignore_patterns = IGNORE_PATTERNS + (ignore if ignore else []) + files = [] + + for root, dirs, filenames in os.walk(resolved): + dirs[:] = [d for d in dirs if not any(fnmatch.fnmatch(d, pat.strip("/")) for pat in ignore_patterns)] + + for filename in filenames: + rel_path = str(Path(root).relative_to(resolved) / filename) + if any(fnmatch.fnmatch(rel_path, pat.strip("/")) for pat in ignore_patterns): + continue + files.append(rel_path) + + if len(files) >= limit: + break if len(files) >= limit: break - if len(files) >= limit: - break + dirs_set = set() + files_by_dir: Dict[str, List[str]] = {} - dirs_set = set() - files_by_dir: Dict[str, List[str]] = {} + for file in files: + dirpath = str(Path(file).parent) + if dirpath == ".": + dirpath = "" - for file in files: - dirpath = str(Path(file).parent) - if dirpath == ".": - dirpath = "" + if dirpath: + dirs_set.add(dirpath) - if dirpath: - dirs_set.add(dirpath) + files_by_dir.setdefault(dirpath, []).append(Path(file).name) - files_by_dir.setdefault(dirpath, []).append(Path(file).name) + current = Path(file).parent + while str(current) != ".": + parent = current.parent + parent_str = str(parent) - current = Path(file).parent - while str(current) != ".": - parent = current.parent - parent_str = str(parent) + if parent_str == "." or parent_str == str(current): + break - if parent_str == "." or parent_str == str(current): - break - - if parent_str and parent_str not in dirs_set: - dirs_set.add(parent_str) + if parent_str and parent_str not in dirs_set: + dirs_set.add(parent_str) - current = parent + current = parent - def render_dir(dirpath: str, depth: int = 0) -> str: - indent = " " * depth - output = "" + def render_dir(dirpath: str, depth: int = 0) -> str: + indent = " " * depth + output = "" - if depth > 0 and dirpath: - output += f"{indent}{Path(dirpath).name}/\n" + if depth > 0 and dirpath: + output += f"{indent}{Path(dirpath).name}/\n" - # Subdirectories - children = sorted([ - d for d in dirs_set - if d and str(Path(d).parent) == (dirpath if dirpath else ".") - ]) + # Subdirectories + children = sorted([ + d for d in dirs_set + if d and str(Path(d).parent) == (dirpath if dirpath else ".") + ]) - for child in children: - output += render_dir(child, depth + 1) + for child in children: + output += render_dir(child, depth + 1) - # Files - for f in sorted(files_by_dir.get(dirpath, [])): - output += f"{' ' * (depth + 1)}{f}\n" + # Files + for f in sorted(files_by_dir.get(dirpath, [])): + output += f"{' ' * (depth + 1)}{f}\n" - return output + return output - output = f"{resolved}/\n" + render_dir("") - truncated = len(files) >= limit + output = f"{resolved}/\n" + render_dir("") + truncated = len(files) >= limit - ctx["result_label"] = f"Listed {len(files)} files in {resolved.name}" - ctx["result_data"] = { - "count": len(files), - "truncated": truncated, - } - - return { - "title": resolved.name, - "metadata": { + ctx["result_label"] = f"Listed {len(files)} files in {resolved.name}" + ctx["result_data"] = { "count": len(files), "truncated": truncated, - }, - "output": output, - } + } + + return { + "title": resolved.name, + "metadata": { + "count": len(files), + "truncated": truncated, + }, + "output": output, + } + + async def _arun( + self, + path: Optional[str] = None, + ignore: Optional[List[str]] = None, + limit: int = 100, + run_manager: Optional[CallbackManagerForToolRun] = None, + ) -> dict: + """Async version - just calls the sync version.""" + return self._run(path, ignore, limit, run_manager) + + +def create_list_tool(working_directory: Optional[str] = None) -> ListTool: + """ + Factory function to create a ListTool instance for LangChain. + + Args: + working_directory: Directory to list by default (default: current directory) + + Returns: + Configured ListTool instance ready for use with LangChain + + Example: + >>> from agent.tools import create_list_tool + >>> list_tool = create_list_tool(working_directory="./project") + >>> # Use with LangChain agent + >>> tools = [list_tool] + """ + return ListTool(working_directory=working_directory) diff --git a/tests/unit/agent/tools/test_glob_tool.py b/tests/unit/agent/tools/test_glob_tool.py index 43d3075..d791c63 100644 --- a/tests/unit/agent/tools/test_glob_tool.py +++ b/tests/unit/agent/tools/test_glob_tool.py @@ -1,6 +1,6 @@ """Unit tests for glob_tool.""" import pytest -from agent.tools.glob_tool import glob_tool +from agent.tools.glob_tool import create_glob_tool class TestGlobTool: @@ -9,6 +9,7 @@ def test_finds_matching_py_files(self, tmp_path): (tmp_path / "utils.py").write_text("code") (tmp_path / "readme.md").write_text("docs") + glob_tool = create_glob_tool() result = glob_tool.invoke({"pattern": "*.py", "path": str(tmp_path)}) assert result["metadata"]["count"] == 2 @@ -21,6 +22,7 @@ def test_finds_matching_txt_files(self, tmp_path): (tmp_path / "todo.txt").write_text("text") (tmp_path / "code.py").write_text("code") + glob_tool = create_glob_tool() result = glob_tool.invoke({"pattern": "*.txt", "path": str(tmp_path)}) assert result["metadata"]["count"] == 2 @@ -30,6 +32,7 @@ def test_finds_matching_txt_files(self, tmp_path): def test_no_matches_returns_zero(self, tmp_path): (tmp_path / "file.py").write_text("code") + glob_tool = create_glob_tool() result = glob_tool.invoke({"pattern": "*.xyz", "path": str(tmp_path)}) assert result["metadata"]["count"] == 0 @@ -43,6 +46,7 @@ def test_recursive_search_exact_filename(self, tmp_path): (sub / "target.py").write_text("code") (deep / "target.py").write_text("code") + glob_tool = create_glob_tool() result = glob_tool.invoke({"pattern": "**/target.py", "path": str(tmp_path)}) assert result["metadata"]["count"] == 2 @@ -52,6 +56,7 @@ def test_limit_truncates_results(self, tmp_path): for i in range(10): (tmp_path / f"file{i}.py").write_text("") + glob_tool = create_glob_tool() result = glob_tool.invoke({"pattern": "*.py", "path": str(tmp_path), "limit": 3}) assert result["metadata"]["truncated"] is True @@ -62,6 +67,41 @@ def test_wildcard_in_name(self, tmp_path): (tmp_path / "other_tool.py").write_text("") (tmp_path / "readme.md").write_text("") + glob_tool = create_glob_tool() result = glob_tool.invoke({"pattern": "*tool*", "path": str(tmp_path)}) - assert result["metadata"]["count"] == 2 \ No newline at end of file + assert result["metadata"]["count"] == 2 + + def test_uses_working_directory_when_path_not_provided(self, tmp_path): + """Test that the tool uses configured working_directory when path is omitted.""" + (tmp_path / "project_file.py").write_text("code") + (tmp_path / "other.txt").write_text("text") + + # Create tool with working_directory set + tool = create_glob_tool(working_directory=str(tmp_path)) + + # Call without path argument - should search working_directory + result = tool.invoke({"pattern": "*.py"}) + + assert result["metadata"]["count"] == 1 + assert "project_file.py" in result["output"] + + def test_explicit_path_overrides_working_directory(self, tmp_path): + """Test that explicit path argument overrides working_directory.""" + project_dir = tmp_path / "project" + project_dir.mkdir() + (project_dir / "project_file.py").write_text("code") + + other_dir = tmp_path / "other" + other_dir.mkdir() + (other_dir / "other_file.py").write_text("code") + + # Create tool with project_dir as working_directory + tool = create_glob_tool(working_directory=str(project_dir)) + + # Call with explicit path to other_dir - should search other_dir + result = tool.invoke({"pattern": "*.py", "path": str(other_dir)}) + + assert result["metadata"]["count"] == 1 + assert "other_file.py" in result["output"] + assert "project_file.py" not in result["output"] \ No newline at end of file diff --git a/tests/unit/agent/tools/test_grep_tool.py b/tests/unit/agent/tools/test_grep_tool.py index fb1f750..fcb8b9f 100644 --- a/tests/unit/agent/tools/test_grep_tool.py +++ b/tests/unit/agent/tools/test_grep_tool.py @@ -1,12 +1,13 @@ """Unit tests for grep_tool.""" import pytest -from agent.tools.grep_tool import grep_tool +from agent.tools.grep_tool import create_grep_tool class TestGrepTool: def test_finds_pattern_in_files(self, tmp_path): (tmp_path / "app.py").write_text("def hello():\n pass\n\ndef world():\n pass\n") + grep_tool = create_grep_tool() result = grep_tool.invoke({"pattern": "def", "path": str(tmp_path), "include": "*.py"}) assert result["metadata"]["count"] > 0 @@ -16,6 +17,7 @@ def test_finds_pattern_in_files(self, tmp_path): def test_no_matches_returns_zero(self, tmp_path): (tmp_path / "app.py").write_text("print('hello')") + grep_tool = create_grep_tool() result = grep_tool.invoke({"pattern": "nonexistentpattern12345", "path": str(tmp_path)}) assert result["metadata"]["count"] == 0 @@ -24,6 +26,7 @@ def test_include_filter(self, tmp_path): (tmp_path / "app.py").write_text("import os") (tmp_path / "notes.txt").write_text("import stuff") + grep_tool = create_grep_tool() result = grep_tool.invoke({"pattern": "import", "path": str(tmp_path), "include": "*.py"}) assert "app.py" in result["output"] @@ -33,6 +36,7 @@ def test_limit_truncates_results(self, tmp_path): lines = "\n".join([f"match line {i}" for i in range(20)]) (tmp_path / "big.txt").write_text(lines) + grep_tool = create_grep_tool() result = grep_tool.invoke({"pattern": "match", "path": str(tmp_path), "limit": 3}) assert result["metadata"]["truncated"] is True @@ -40,6 +44,7 @@ def test_limit_truncates_results(self, tmp_path): def test_regex_pattern(self, tmp_path): (tmp_path / "code.py").write_text("class MyClass:\n x = 42\n\nclass Other:\n y = 7\n") + grep_tool = create_grep_tool() result = grep_tool.invoke({"pattern": r"class \w+:", "path": str(tmp_path)}) assert result["metadata"]["count"] == 2 @@ -49,6 +54,7 @@ def test_searches_subdirectories(self, tmp_path): sub.mkdir() (sub / "nested.py").write_text("def nested_func():\n pass\n") + grep_tool = create_grep_tool() result = grep_tool.invoke({"pattern": "nested_func", "path": str(tmp_path)}) assert result["metadata"]["count"] > 0 @@ -58,7 +64,43 @@ def test_skips_binary_files(self, tmp_path): (tmp_path / "image.png").write_bytes(b"\x89PNG\r\n") (tmp_path / "code.py").write_text("findme here") + grep_tool = create_grep_tool() result = grep_tool.invoke({"pattern": "findme", "path": str(tmp_path)}) assert "code.py" in result["output"] - assert "image.png" not in result["output"] \ No newline at end of file + assert "image.png" not in result["output"] + + def test_uses_working_directory_when_path_not_provided(self, tmp_path): + """Test that the tool uses configured working_directory when path is omitted.""" + (tmp_path / "project_file.py").write_text("searchterm here") + (tmp_path / "other.txt").write_text("text") + + # Create tool with working_directory set + tool = create_grep_tool(working_directory=str(tmp_path)) + + # Call without path argument - should search working_directory + result = tool.invoke({"pattern": "searchterm"}) + + assert result["metadata"]["count"] == 1 + assert "project_file.py" in result["output"] + + def test_explicit_path_overrides_working_directory(self, tmp_path): + """Test that explicit path argument overrides working_directory.""" + project_dir = tmp_path / "project" + project_dir.mkdir() + (project_dir / "project_file.py").write_text("findme in project") + + other_dir = tmp_path / "other" + other_dir.mkdir() + (other_dir / "other_file.py").write_text("findme in other") + + # Create tool with project_dir as working_directory + tool = create_grep_tool(working_directory=str(project_dir)) + + # Call with explicit path to other_dir - should search other_dir + result = tool.invoke({"pattern": "findme", "path": str(other_dir)}) + + assert result["metadata"]["count"] == 1 + assert "other_file.py" in result["output"] + assert "in other" in result["output"] + assert "project_file.py" not in result["output"] \ No newline at end of file diff --git a/tests/unit/agent/tools/test_list_tool.py b/tests/unit/agent/tools/test_list_tool.py index 5fbfa10..e8357e3 100644 --- a/tests/unit/agent/tools/test_list_tool.py +++ b/tests/unit/agent/tools/test_list_tool.py @@ -1,6 +1,6 @@ """Unit tests for list_tool.""" import pytest -from agent.tools.list_tool import list_tool +from agent.tools.list_tool import create_list_tool class TestListTool: @@ -11,6 +11,7 @@ def test_lists_directory_contents(self, tmp_path): sub.mkdir() (sub / "nested.py").write_text("") + list_tool = create_list_tool() result = list_tool.invoke({"path": str(tmp_path)}) assert "file1.py" in result["output"] @@ -21,6 +22,7 @@ def test_ignore_pattern(self, tmp_path): (tmp_path / "keep.py").write_text("") (tmp_path / "skip.txt").write_text("") + list_tool = create_list_tool() result = list_tool.invoke({"path": str(tmp_path), "ignore": ["*.txt"]}) assert "keep.py" in result["output"] @@ -31,6 +33,7 @@ def test_multiple_ignore_patterns(self, tmp_path): (tmp_path / "skip.py").write_text("") (tmp_path / "skip.txt").write_text("") + list_tool = create_list_tool() result = list_tool.invoke({"path": str(tmp_path), "ignore": ["*.py", "*.txt"]}) assert "keep.md" in result["output"] @@ -41,6 +44,7 @@ def test_limit_truncates(self, tmp_path): for i in range(20): (tmp_path / f"file{i}.txt").write_text("") + list_tool = create_list_tool() result = list_tool.invoke({"path": str(tmp_path), "limit": 5}) assert result["metadata"]["truncated"] is True @@ -49,11 +53,48 @@ def test_large_limit_no_truncation(self, tmp_path): (tmp_path / "one.py").write_text("") (tmp_path / "two.py").write_text("") + list_tool = create_list_tool() result = list_tool.invoke({"path": str(tmp_path), "limit": 1000}) assert result["metadata"]["truncated"] is False def test_empty_directory(self, tmp_path): + list_tool = create_list_tool() result = list_tool.invoke({"path": str(tmp_path)}) - assert result["metadata"]["count"] == 0 \ No newline at end of file + assert result["metadata"]["count"] == 0 + + def test_uses_working_directory_when_path_not_provided(self, tmp_path): + """Test that the tool uses configured working_directory when path is omitted.""" + (tmp_path / "project_file.py").write_text("code") + (tmp_path / "other.txt").write_text("text") + + # Create tool with working_directory set + tool = create_list_tool(working_directory=str(tmp_path)) + + # Call without path argument - should list working_directory + result = tool.invoke({}) + + assert result["metadata"]["count"] == 2 + assert "project_file.py" in result["output"] + assert "other.txt" in result["output"] + + def test_explicit_path_overrides_working_directory(self, tmp_path): + """Test that explicit path argument overrides working_directory.""" + project_dir = tmp_path / "project" + project_dir.mkdir() + (project_dir / "project_file.py").write_text("code") + + other_dir = tmp_path / "other" + other_dir.mkdir() + (other_dir / "other_file.py").write_text("code") + + # Create tool with project_dir as working_directory + tool = create_list_tool(working_directory=str(project_dir)) + + # Call with explicit path to other_dir - should list other_dir + result = tool.invoke({"path": str(other_dir)}) + + assert result["metadata"]["count"] == 1 + assert "other_file.py" in result["output"] + assert "project_file.py" not in result["output"] \ No newline at end of file diff --git a/tests/unit/agent/tools/test_streaming_instrumentation.py b/tests/unit/agent/tools/test_streaming_instrumentation.py index adcfa46..35eb3a8 100644 --- a/tests/unit/agent/tools/test_streaming_instrumentation.py +++ b/tests/unit/agent/tools/test_streaming_instrumentation.py @@ -6,9 +6,9 @@ from agent.exceptions import PermissionRequiredException from agent.tools.bash_tool import create_bash_tool -from agent.tools.glob_tool import glob_tool -from agent.tools.grep_tool import grep_tool -from agent.tools.list_tool import list_tool +from agent.tools.glob_tool import create_glob_tool +from agent.tools.grep_tool import create_grep_tool +from agent.tools.list_tool import create_list_tool from agent.tools.read_tool import read from agent.tools.write_tool import write @@ -33,9 +33,9 @@ def test_successful_tools_emit_started_and_completed( source.write_text("def hello():\n return 'hello'", encoding="utf-8") read.invoke({"filePath": str(source)}) - grep_tool.invoke({"pattern": "return", "path": str(tmp_path), "include": "*.py"}) - glob_tool.invoke({"pattern": "*.py", "path": str(tmp_path)}) - list_tool.invoke({"path": str(tmp_path)}) + create_grep_tool(working_directory=str(tmp_path)).invoke({"pattern": "return", "include": "*.py"}) + create_glob_tool(working_directory=str(tmp_path)).invoke({"pattern": "*.py"}) + create_list_tool(working_directory=str(tmp_path)).invoke({}) write.invoke({"filePath": str(tmp_path / "out.txt"), "content": "done"}) create_bash_tool(working_directory=str(tmp_path))._run("echo hello") From 1c069f2f34b45ba7203613fd46f31c99d41a559a Mon Sep 17 00:00:00 2001 From: Hemdan47 Date: Mon, 22 Jun 2026 06:01:58 +0300 Subject: [PATCH 3/8] fix: (planner) prevent duplicate title emissions and state updates Bug: The planner node emitted the title event but failed to persist it to state when branching to analyzer (Branch 1) or human interaction (Branch 2). On subsequent executions, Branch 3 would unconditionally overwrite the title field, potentially causing duplicate emissions or title changes during replans. Fix: Track when title is newly generated with a flag and conditionally include it in state updates only for that execution. This ensures the title is emitted once, persisted immediately, and never overwritten in subsequent planner calls. --- src/agent/graph/nodes/planner.py | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/agent/graph/nodes/planner.py b/src/agent/graph/nodes/planner.py index 42adc3b..5d65037 100644 --- a/src/agent/graph/nodes/planner.py +++ b/src/agent/graph/nodes/planner.py @@ -162,15 +162,21 @@ def planner_node(state: State) -> dict: emit_update(response.public_update) # Emit title early if it was just generated + title_was_just_generated = False if not state.get("title", "") and response.title: emit_title(response.title) + title_was_just_generated = True # --- Branch 1: Analyzer context needed --- if response.needs_analysis and response.analyzer_query: - return { + updates = { "analyzer_query": response.analyzer_query, "query_requester": "planner", } + # Include title in state if it was just generated + if title_was_just_generated: + updates["title"] = response.title + return updates # --- Branch 2: direct human clarification --- if response.needs_human_input and response.human_question: @@ -179,20 +185,26 @@ def planner_node(state: State) -> dict: "source": "planner", "message": response.human_question, } - return { + updates = { "pending_question": question } + # Include title in state if it was just generated + if title_was_just_generated: + updates["title"] = response.title + return updates # --- Branch 3: Produce the plan --- updates: dict = { "pipeline_plan": response.pipeline_steps, "planned_filepath": response.planned_filepath, - "title": response.title, # Clear the handshake fields only — keep analyzer_result so the # Generator can also benefit from the accumulated analysis history "analyzer_query": "", "query_requester": "", } + # Include title in state only if it was just generated + if title_was_just_generated: + updates["title"] = response.title # Track empty plan attempts for deadlock prevention if not response.pipeline_steps: From ba2970da975bfed8b19164c139459ecf1e582ba4 Mon Sep 17 00:00:00 2001 From: Hemdan47 Date: Tue, 23 Jun 2026 00:36:21 +0300 Subject: [PATCH 4/8] feat!: migrate to per-request LLM configuration via LiteLLM Problem: Agent was coupled to environment variables for LLM config, preventing per-request model selection and requiring server restart for changes. Custom models only worked with OpenAI provider. Solution: - Replace env-based config with per-request LLMConfig schema - Route all providers through LiteLLM (openai, anthropic, gemini, groq, openai_compatible) - Add shared get_llm_from_config() helper in llm/helpers.py - Update nodes to accept RunnableConfig and extract llm_config - Remove langchain-{groq,google-genai,openai}, add langchain-litellm --- pyproject.toml | 4 +- src/agent/api/routes/health.py | 5 +- src/agent/api/routes/streaming.py | 14 +- src/agent/api/schemas/health.py | 2 - src/agent/api/schemas/streaming.py | 48 ++++- src/agent/config.py | 50 ----- src/agent/graph/nodes/planner.py | 7 +- src/agent/graph/subgraphs/analyzer.py | 12 +- .../generator_reflexion/generator.py | 7 +- src/agent/llm/factory.py | 83 ++++++++ src/agent/llm/helpers.py | 48 +++++ src/agent/llm/provider.py | 42 ---- .../agent/api/test_streaming_routes.py | 41 +++- .../agent/graph/nodes/test_planner.py | 51 ++++- .../agent/graph/subgraphs/test_analyzer.py | 47 +++++ .../integration/agent/graph/test_hitl_flow.py | 47 ++++- .../agent/graph/subgraphs/test_analyzer.py | 26 ++- tests/unit/agent/llm/__init__.py | 1 + tests/unit/agent/llm/test_factory.py | 181 ++++++++++++++++++ 19 files changed, 583 insertions(+), 133 deletions(-) delete mode 100644 src/agent/config.py create mode 100644 src/agent/llm/factory.py create mode 100644 src/agent/llm/helpers.py delete mode 100644 src/agent/llm/provider.py create mode 100644 tests/unit/agent/llm/__init__.py create mode 100644 tests/unit/agent/llm/test_factory.py diff --git a/pyproject.toml b/pyproject.toml index 5bb57ff..8eebd9a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -12,9 +12,7 @@ dependencies = [ "langgraph>=1.1.10,<1.2", # LLM Providers - "langchain-groq>=0.1.0", - "langchain-google-genai>=1.0.0", - "langchain-openai", + "langchain-litellm", # Tools "requests>=2.31.0", diff --git a/src/agent/api/routes/health.py b/src/agent/api/routes/health.py index dd1ab6e..1fed254 100644 --- a/src/agent/api/routes/health.py +++ b/src/agent/api/routes/health.py @@ -1,15 +1,12 @@ from fastapi import APIRouter from agent.api.schemas import HealthResponse -from agent.config import get_llm_provider, get_generation_model router = APIRouter(tags=["Health"]) @router.get("/health", response_model=HealthResponse) async def health_check(): - """Health check + current LLM configuration.""" + """Health check endpoint.""" return HealthResponse( status="healthy", version="1.0.0", - provider=get_llm_provider(), - model=get_generation_model(), ) diff --git a/src/agent/api/routes/streaming.py b/src/agent/api/routes/streaming.py index 02a9451..043a9f1 100644 --- a/src/agent/api/routes/streaming.py +++ b/src/agent/api/routes/streaming.py @@ -21,7 +21,12 @@ async def generate_stream(request: StreamRequest): """Start a new streaming workflow generation.""" thread_id = request.thread_id run_id = f"run_{uuid4().hex[:12]}" - config = {"configurable": {"thread_id": thread_id}} + config = { + "configurable": { + "thread_id": thread_id, + "llm_config": request.llm.model_dump(), + } + } initial_state = { "messages": build_messages(request.history, request.prompt), @@ -71,7 +76,12 @@ async def generate_stream(request: StreamRequest): async def resume_stream(thread_id: str, request: ResumeRequest): """Resume an interrupted workflow generation stream.""" run_id = f"run_{uuid4().hex[:12]}" - config = {"configurable": {"thread_id": thread_id}} + config = { + "configurable": { + "thread_id": thread_id, + "llm_config": request.llm.model_dump(), + } + } return StreamingResponse( stream_graph( diff --git a/src/agent/api/schemas/health.py b/src/agent/api/schemas/health.py index 26dd09a..db52b17 100644 --- a/src/agent/api/schemas/health.py +++ b/src/agent/api/schemas/health.py @@ -5,5 +5,3 @@ class HealthResponse(BaseModel): status: str version: str - provider: str - model: str diff --git a/src/agent/api/schemas/streaming.py b/src/agent/api/schemas/streaming.py index 6965f75..3c9b268 100644 --- a/src/agent/api/schemas/streaming.py +++ b/src/agent/api/schemas/streaming.py @@ -2,10 +2,52 @@ from __future__ import annotations -from typing import Literal +from typing import Literal, Optional from langchain_core.messages import AIMessage, BaseMessage, HumanMessage -from pydantic import BaseModel, Field +from pydantic import BaseModel, Field, model_validator + + +Provider = Literal[ + "openai", + "anthropic", + "gemini", # Google Generative AI + "groq", + "openai_compatible", # vLLM, Ollama /v1, LM Studio, OpenRouter, Together, ngrok, … +] + + +class LLMConfig(BaseModel): + """Fully-resolved LLM settings supplied by the caller. REQUIRED on every request.""" + provider: Provider = Field(..., description="LiteLLM provider discriminator.") + model: str = Field(..., description="Model name, e.g. 'gpt-4o', " + "'claude-3-5-sonnet-20241022', 'llama-3.3-70b-versatile'.") + api_key: Optional[str] = Field( + default=None, + description="API key. Required for openai, anthropic, gemini, groq. " + "Optional for openai_compatible (local servers may need none; " + "hosted services like OpenRouter still require one). " + "Never logged.", + ) + base_url: Optional[str] = Field( + default=None, + description="Custom base URL. REQUIRED for 'openai_compatible' " + "(must be OpenAI-shaped, i.e. expose /v1/...). " + "Optional override for the first-class providers.", + ) + temperature: float = Field(default=0, description="Sampling temperature. 0 = " + "deterministic, the right default for structured " + "YAML generation. Caller can override per request.") + + @model_validator(mode="after") + def _validate_provider_requirements(self) -> "LLMConfig": + if self.provider == "openai_compatible": + if not self.base_url: + raise ValueError("provider='openai_compatible' requires base_url") + else: + if not self.api_key: + raise ValueError(f"provider='{self.provider}' requires api_key") + return self class MessageItem(BaseModel): @@ -47,9 +89,11 @@ class StreamRequest(BaseModel): default="", description="Current thread title. Empty on first turn, agent generates one.", ) + llm: LLMConfig = Field(..., description="Per-request LLM configuration (required).") class ResumeRequest(BaseModel): """Request body for resuming an interrupted stream.""" response: str = Field(..., description="User answer to the pending question") + llm: LLMConfig = Field(..., description="Per-request LLM configuration (required).") diff --git a/src/agent/config.py b/src/agent/config.py deleted file mode 100644 index 63efd94..0000000 --- a/src/agent/config.py +++ /dev/null @@ -1,50 +0,0 @@ -""" -Configuration for the CI/CD Workflow Generator. -Loads settings from environment variables with sensible defaults. -""" - -from __future__ import annotations - -import os -from typing import Optional - -from dotenv import load_dotenv - -load_dotenv() - -# --- API Key Resolution --- - -_KEY_ENV_VARS = { - "groq": "GROQ_API_KEY", - "openai": "OPENAI_API_KEY", - "google_genai": "GOOGLE_API_KEY", - "anthropic": "ANTHROPIC_API_KEY", -} - - -def get_api_key(provider: str) -> Optional[str]: - """Get the API key for the given provider from environment variables.""" - env_var = _KEY_ENV_VARS.get(provider) - return os.getenv(env_var) if env_var else None - - -# --- App Config --- - -def get_llm_provider() -> str: - """Return the configured LLM provider name.""" - return os.getenv("LLM_PROVIDER", "groq") - - -def get_generation_model() -> str: - """Return the configured generation model name.""" - return os.getenv("GENERATION_MODEL", "llama-3.3-70b-versatile") - - -def get_temperature() -> float: - """Return the configured LLM temperature.""" - return float(os.getenv("LLM_TEMPERATURE", "0")) - - -def get_api_base() -> Optional[str]: - """Return the configured API base URL (e.g. ngrok URL for local vLLM).""" - return os.getenv("OPENAI_API_BASE") diff --git a/src/agent/graph/nodes/planner.py b/src/agent/graph/nodes/planner.py index 5d65037..76fbb2b 100644 --- a/src/agent/graph/nodes/planner.py +++ b/src/agent/graph/nodes/planner.py @@ -10,12 +10,13 @@ from __future__ import annotations from langchain_core.messages import HumanMessage, SystemMessage +from langchain_core.runnables import RunnableConfig from agent.graph.constants import MAX_REPLANS, MAX_EMPTY_PLAN_ATTEMPTS from agent.graph.prompts import PLANNER_SYSTEM_PROMPT from agent.graph.schemas import PlannerOutput, PendingQuestion from agent.graph.state import State -from agent.llm.provider import get_generation_llm +from agent.llm.helpers import get_llm_from_config from agent.streaming.emitter import emit_plan, emit_title, emit_update @@ -97,7 +98,7 @@ def _build_deadlock_message(state: State) -> str: return "\n".join(parts) -def planner_node(state: State) -> dict: +def planner_node(state: State, config: RunnableConfig) -> dict: """ LangGraph node: plans the CI/CD pipeline or requests Analyzer context. @@ -138,7 +139,7 @@ def planner_node(state: State) -> dict: "replan_count": 0, } - llm = get_generation_llm() + llm = get_llm_from_config(config) structured_llm = llm.with_structured_output(PlannerOutput, method="function_calling") target_platform = state.get("target_platform", "github_actions") diff --git a/src/agent/graph/subgraphs/analyzer.py b/src/agent/graph/subgraphs/analyzer.py index 6df321d..3ca9737 100644 --- a/src/agent/graph/subgraphs/analyzer.py +++ b/src/agent/graph/subgraphs/analyzer.py @@ -11,11 +11,12 @@ from langchain.agents import create_agent from langchain_core.messages import HumanMessage +from langchain_core.runnables import RunnableConfig from agent.graph.prompts import ANALYZER_SYSTEM_PROMPT from agent.graph.schemas import AnalyzerResultEntry, PendingQuestion from agent.graph.state import State -from agent.llm.provider import get_generation_llm +from agent.llm.helpers import get_llm_from_config from agent.tools.permission.protocol import parse_allow_deny from agent.exceptions import PermissionRequiredException from agent.tools import ( @@ -61,7 +62,7 @@ def build_analyzer_agent(project_path: str, llm=None, approved_commands: set[str Args: project_path: Absolute path to the project to analyze. - llm: LangChain chat model. Defaults to the generation LLM. + llm: LangChain chat model. If None, raises an error (no default). Returns: A compiled LangGraph (has ``.invoke()``). @@ -71,7 +72,7 @@ def build_analyzer_agent(project_path: str, llm=None, approved_commands: set[str project_path = project_path.replace("\\", "/") if llm is None: - llm = get_generation_llm() + raise ValueError("build_analyzer_agent requires an llm parameter") tools = [ create_bash_tool( @@ -124,7 +125,7 @@ def run_analyzer( } -def analyzer_node(state: State) -> dict: +def analyzer_node(state: State, config: RunnableConfig) -> dict: """Parent graph node: reads query from state, runs analyzer, writes result. Registered as: ``graph.add_node("analyzer", analyzer_node)`` @@ -138,6 +139,8 @@ def analyzer_node(state: State) -> dict: if not project_path: raise ValueError("analyzer_node requires 'project_path' in state") + llm = get_llm_from_config(config) + permission_request = state.get("pending_permission_request") or {} approved_commands: set[str] = set(state.get("approved_commands", [])) @@ -181,6 +184,7 @@ def analyzer_node(state: State) -> dict: result = run_analyzer( project_path=project_path, query=query, + llm=llm, approved_commands=approved_commands, ) diff --git a/src/agent/graph/subgraphs/generator_reflexion/generator.py b/src/agent/graph/subgraphs/generator_reflexion/generator.py index 823e713..9c07489 100644 --- a/src/agent/graph/subgraphs/generator_reflexion/generator.py +++ b/src/agent/graph/subgraphs/generator_reflexion/generator.py @@ -10,6 +10,7 @@ from __future__ import annotations from langchain_core.messages import HumanMessage, SystemMessage +from langchain_core.runnables import RunnableConfig from agent.graph.prompts import ( GENERATOR_SYSTEM_PROMPT, @@ -17,7 +18,7 @@ ) from agent.graph.schemas import GeneratorOutput, PendingQuestion from agent.graph.state import State -from agent.llm.provider import get_generation_llm +from agent.llm.helpers import get_llm_from_config from agent.streaming.emitter import emit_update from agent.utils import clean_yaml_fences @@ -73,12 +74,12 @@ def _build_user_message(state: State) -> str: return "\n\n".join(parts) if parts else "Generate a CI/CD workflow." -def generator_node(state: State) -> dict: +def generator_node(state: State, config: RunnableConfig) -> dict: """LangGraph node: generates or refines a CI/CD YAML draft. Returns a partial state dict — only the fields this node writes. """ - llm = get_generation_llm() + llm = get_llm_from_config(config) structured_llm = llm.with_structured_output(GeneratorOutput, method="function_calling") target_platform = state.get("target_platform", "github_actions") diff --git a/src/agent/llm/factory.py b/src/agent/llm/factory.py new file mode 100644 index 0000000..33c4e97 --- /dev/null +++ b/src/agent/llm/factory.py @@ -0,0 +1,83 @@ +""" +LLM Factory — unified construction via LiteLLM. + +All provider-specific logic (API keys, model aliases, client setup) +is handled by LiteLLM internally. We expose one function: build_llm. +""" + +from __future__ import annotations + +from functools import lru_cache +from typing import TYPE_CHECKING + +from langchain_core.language_models import BaseChatModel +from langchain_litellm import ChatLiteLLM +from agent.api.schemas.streaming import LLMConfig +if TYPE_CHECKING: + from agent.api.schemas.streaming import LLMConfig + + +_PROVIDER_PREFIX = { + "openai": "openai", + "anthropic": "anthropic", + "gemini": "gemini", # LiteLLM: gemini/ + "groq": "groq", + "openai_compatible": "openai", # ChatLiteLLM treats as OpenAI-shaped + base_url +} + + +def _litellm_model(cfg: "LLMConfig") -> str: + """Convert our provider discriminator to LiteLLM's model string.""" + return f"{_PROVIDER_PREFIX[cfg.provider]}/{cfg.model}" + + +def _make_hashable_config(cfg: "LLMConfig") -> tuple: + """Convert LLMConfig to a hashable tuple for caching.""" + return ( + cfg.provider, + cfg.model, + cfg.api_key or "", + cfg.base_url or "", + cfg.temperature, + ) + + +@lru_cache(maxsize=32) +def _cached_build_llm(cfg_tuple: tuple) -> BaseChatModel: + """Internal cached builder. Do not call directly.""" + # Reconstruct a minimal config-like object + + provider, model, api_key, base_url, temperature = cfg_tuple + cfg = LLMConfig( + provider=provider, + model=model, + api_key=api_key or None, + base_url=base_url or None, + temperature=temperature, + ) + + kwargs = { + "model": _litellm_model(cfg), + "temperature": cfg.temperature, + } + if cfg.api_key: + kwargs["api_key"] = cfg.api_key + if cfg.base_url: + kwargs["api_base"] = cfg.base_url # LiteLLM's param is named api_base + + return ChatLiteLLM(**kwargs) + + +def build_llm(cfg: "LLMConfig") -> BaseChatModel: + """Build a LangChain chat model from the given config. + + Uses LRU caching so the three nodes in one request share one constructed client. + Cache is capped at 32 entries to bound memory in long-running servers. + + Args: + cfg: Fully-resolved LLM configuration from the request. + + Returns: + A LangChain BaseChatModel ready for .invoke() or .with_structured_output(). + """ + return _cached_build_llm(_make_hashable_config(cfg)) diff --git a/src/agent/llm/helpers.py b/src/agent/llm/helpers.py new file mode 100644 index 0000000..831fb0d --- /dev/null +++ b/src/agent/llm/helpers.py @@ -0,0 +1,48 @@ +"""Helper utilities for LLM access in graph nodes.""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +from langchain_core.language_models import BaseChatModel +from langchain_core.runnables import RunnableConfig + +from agent.api.schemas.streaming import LLMConfig +from agent.llm.factory import build_llm + +if TYPE_CHECKING: + pass + + +def get_llm_from_config(config: RunnableConfig) -> BaseChatModel: + """Extract LLMConfig from RunnableConfig and build the LLM. + + This is the standard way for graph nodes to access the LLM. The config + is populated by the API route from the incoming request's `llm` field. + + Args: + config: LangGraph's RunnableConfig, expected to have + config["configurable"]["llm_config"] populated. + + Returns: + A LangChain BaseChatModel instance ready for use. + + Raises: + ValueError: If llm_config is missing from the config. + + Example: + ```python + def my_node(state: State, config: RunnableConfig) -> dict: + llm = get_llm_from_config(config) + structured_llm = llm.with_structured_output(MySchema) + result = structured_llm.invoke(...) + return {"field": result.field} + ``` + """ + cfg_dict = (config or {}).get("configurable", {}).get("llm_config") + if cfg_dict is None: + raise ValueError( + "llm_config missing from request config. " + "Ensure the API route passes request.llm to config['configurable']['llm_config']" + ) + return build_llm(LLMConfig(**cfg_dict)) diff --git a/src/agent/llm/provider.py b/src/agent/llm/provider.py deleted file mode 100644 index bf0a2a5..0000000 --- a/src/agent/llm/provider.py +++ /dev/null @@ -1,42 +0,0 @@ -""" -LLM Provider — thin wrapper around ``init_chat_model``. - -All provider-specific logic (API keys, model aliases, client setup) -is handled by LangChain internally. We only expose one function. -""" - -from __future__ import annotations - -from langchain.chat_models import init_chat_model -from langchain_core.language_models import BaseChatModel - -from agent.config import get_api_base, get_api_key, get_generation_model, get_llm_provider, get_temperature - - -def get_generation_llm() -> BaseChatModel: - """Create the generation LLM from environment config. - - Reads: LLM_PROVIDER, GENERATION_MODEL, LLM_TEMPERATURE, and - the provider-specific API key env var. - - Returns: - A LangChain chat model ready for ``.invoke()`` or - ``.with_structured_output()``. - """ - provider = get_llm_provider().strip().lower() - model = get_generation_model() - temperature = get_temperature() - api_key = get_api_key(provider) - api_base = get_api_base() - - kwargs: dict = { - "model": model, - "model_provider": provider, - "temperature": temperature, - } - if api_key: - kwargs["api_key"] = api_key - if api_base and provider == "openai": - kwargs["base_url"] = api_base - - return init_chat_model(**kwargs) diff --git a/tests/integration/agent/api/test_streaming_routes.py b/tests/integration/agent/api/test_streaming_routes.py index a92b14d..5010dcb 100644 --- a/tests/integration/agent/api/test_streaming_routes.py +++ b/tests/integration/agent/api/test_streaming_routes.py @@ -74,6 +74,13 @@ async def test_generate_stream_endpoint_returns_ndjson_headers_and_events( "project_path": "D:/repo", "prompt": "Create CI for this Python project", "target_platform": "github_actions", + "thread_id": "test-thread-123", + "llm": { + "provider": "openai", + "model": "gpt-4o", + "api_key": "test-key", + "temperature": 0, + }, }, ) @@ -83,7 +90,16 @@ async def test_generate_stream_endpoint_returns_ndjson_headers_and_events( assert response.headers["x-run-id"].startswith("run_") assert fake_graph.config == { - "configurable": {"thread_id": response.headers["x-thread-id"]} + "configurable": { + "thread_id": response.headers["x-thread-id"], + "llm_config": { + "provider": "openai", + "model": "gpt-4o", + "api_key": "test-key", + "base_url": None, + "temperature": 0, + }, + } } assert fake_graph.kwargs == { "stream_mode": ["updates", "custom"], @@ -129,7 +145,15 @@ async def test_resume_stream_endpoint_uses_command_and_skips_run_started( async with httpx.AsyncClient(transport=transport, base_url="http://test") as client: response = await client.post( "/generate/thread-123/stream/resume", - json={"response": "allow"}, + json={ + "response": "allow", + "llm": { + "provider": "openai", + "model": "gpt-4o", + "api_key": "test-key", + "temperature": 0, + }, + }, ) assert response.status_code == 200 @@ -137,7 +161,18 @@ async def test_resume_stream_endpoint_uses_command_and_skips_run_started( assert response.headers["x-thread-id"] == "thread-123" assert response.headers["x-run-id"].startswith("run_") assert getattr(fake_graph.graph_input, "resume") == "allow" - assert fake_graph.config == {"configurable": {"thread_id": "thread-123"}} + assert fake_graph.config == { + "configurable": { + "thread_id": "thread-123", + "llm_config": { + "provider": "openai", + "model": "gpt-4o", + "api_key": "test-key", + "base_url": None, + "temperature": 0, + }, + } + } events = _parse_ndjson(response) assert [event["seq"] for event in events] == [1] diff --git a/tests/integration/agent/graph/nodes/test_planner.py b/tests/integration/agent/graph/nodes/test_planner.py index b53b3bb..2ef83ff 100644 --- a/tests/integration/agent/graph/nodes/test_planner.py +++ b/tests/integration/agent/graph/nodes/test_planner.py @@ -31,6 +31,43 @@ # Helpers # --------------------------------------------------------------------------- +def _get_llm_config() -> dict: + """Return LLM config based on available API keys.""" + if os.environ.get("GROQ_API_KEY"): + return { + "provider": "groq", + "model": "llama-3.3-70b-versatile", + "api_key": os.environ["GROQ_API_KEY"], + "temperature": 0, + } + elif os.environ.get("OPENAI_API_KEY"): + return { + "provider": "openai", + "model": "gpt-4o", + "api_key": os.environ["OPENAI_API_KEY"], + "temperature": 0, + } + elif os.environ.get("GOOGLE_API_KEY"): + return { + "provider": "gemini", + "model": "gemini-1.5-pro", + "api_key": os.environ["GOOGLE_API_KEY"], + "temperature": 0, + } + elif os.environ.get("ANTHROPIC_API_KEY"): + return { + "provider": "anthropic", + "model": "claude-3-5-sonnet-20241022", + "api_key": os.environ["ANTHROPIC_API_KEY"], + "temperature": 0, + } + raise ValueError("No API key available") + + +def _config() -> dict: + """Return a minimal RunnableConfig with llm_config.""" + return {"configurable": {"llm_config": _get_llm_config()}} + def _base_state(**overrides) -> dict: """Return a minimal valid State dict for the planner.""" state = { @@ -77,7 +114,7 @@ def test_requests_analysis_for_vague_project(self): state = _base_state( messages=[HumanMessage(content="Generate a CI/CD pipeline for my project.")], ) - result = planner_node(state) + result = planner_node(state, _config()) # If the LLM correctly decides it needs more info # it should set analyzer_query and query_requester @@ -97,7 +134,7 @@ def test_produces_plan_for_clear_request(self): ) )], ) - result = planner_node(state) + result = planner_node(state, _config()) # With enough context the planner should produce a plan if not result.get("analyzer_query"): @@ -129,7 +166,7 @@ def test_github_actions_python_plan(self): "Do not request any further analysis, build the pipeline now." )], ) - result = planner_node(state) + result = planner_node(state, _config()) # Should not request further analysis since analyzer_result is provided assert not result.get("analyzer_query"), ( @@ -155,7 +192,7 @@ def test_gitlab_ci_node_plan(self): "Do not request any further analysis, build the pipeline now." )], ) - result = planner_node(state) + result = planner_node(state, _config()) assert not result.get("analyzer_query") plan = result.get("pipeline_plan", []) @@ -169,7 +206,7 @@ def test_clears_handshake_fields_after_planning(self): analyzer_result=[_analyzer_entry("Project analysis", "Language: Go. Build tool: go build. Test: go test.")], query_requester="planner", ) - result = planner_node(state) + result = planner_node(state, _config()) if not result.get("analyzer_query"): # Plan was produced → only handshake fields must be cleared @@ -199,7 +236,7 @@ def test_increments_replan_count(self): replan_reason="The plan is missing a cross-compilation step for Linux arm64.", replan_count=0, ) - result = planner_node(state) + result = planner_node(state, _config()) # Whether it plans or re-requests analysis, replan bookkeeping must happen if not result.get("analyzer_query"): @@ -220,7 +257,7 @@ def test_addresses_replan_reason(self): replan_reason="Missing cross-compilation step for arm64 target.", replan_count=0, ) - result = planner_node(state) + result = planner_node(state, _config()) if not result.get("analyzer_query"): plan = result.get("pipeline_plan", []) diff --git a/tests/integration/agent/graph/subgraphs/test_analyzer.py b/tests/integration/agent/graph/subgraphs/test_analyzer.py index 9c6d02f..13f2e48 100644 --- a/tests/integration/agent/graph/subgraphs/test_analyzer.py +++ b/tests/integration/agent/graph/subgraphs/test_analyzer.py @@ -13,6 +13,8 @@ import time from agent.graph.subgraphs.analyzer import run_analyzer +from agent.llm.factory import build_llm +from agent.api.schemas.streaming import LLMConfig # Skip all tests if no API key is set @@ -25,6 +27,41 @@ ) +def _get_llm(): + """Return an LLM instance based on available API keys.""" + if os.environ.get("GROQ_API_KEY"): + config = LLMConfig( + provider="groq", + model="llama-3.3-70b-versatile", + api_key=os.environ["GROQ_API_KEY"], + temperature=0, + ) + elif os.environ.get("OPENAI_API_KEY"): + config = LLMConfig( + provider="openai", + model="gpt-4o", + api_key=os.environ["OPENAI_API_KEY"], + temperature=0, + ) + elif os.environ.get("GOOGLE_API_KEY"): + config = LLMConfig( + provider="gemini", + model="gemini-1.5-pro", + api_key=os.environ["GOOGLE_API_KEY"], + temperature=0, + ) + elif os.environ.get("ANTHROPIC_API_KEY"): + config = LLMConfig( + provider="anthropic", + model="claude-3-5-sonnet-20241022", + api_key=os.environ["ANTHROPIC_API_KEY"], + temperature=0, + ) + else: + raise ValueError("No API key available") + return build_llm(config) + + @pytest.fixture(autouse=True) def _rate_limit_delay(): """Automatically sleep before every test to prevent LLM Tokens-Per-Minute rate limits (429 errors).""" @@ -132,6 +169,7 @@ def test_detects_python(self, python_flask_project): result = run_analyzer( str(python_flask_project), "What programming language is this project written in?", + llm=_get_llm(), ) assert "python" in result["answer"].lower() @@ -139,6 +177,7 @@ def test_detects_node(self, node_express_project): result = run_analyzer( str(node_express_project), "What programming language is this project written in?", + llm=_get_llm(), ) assert "javascript" in result["answer"].lower() or "node" in result["answer"].lower() @@ -146,6 +185,7 @@ def test_detects_go(self, go_project): result = run_analyzer( str(go_project), "What programming language is this project written in?", + llm=_get_llm(), ) assert "go" in result["answer"].lower() @@ -161,6 +201,7 @@ def test_finds_flask(self, python_flask_project): result = run_analyzer( str(python_flask_project), "What web framework does this project use?", + llm=_get_llm(), ) assert "flask" in result["answer"].lower() @@ -168,6 +209,7 @@ def test_finds_express(self, node_express_project): result = run_analyzer( str(node_express_project), "What web framework does this project use?", + llm=_get_llm(), ) assert "express" in result["answer"].lower() @@ -175,6 +217,7 @@ def test_finds_test_framework(self, python_flask_project): result = run_analyzer( str(python_flask_project), "What test framework is configured in this project?", + llm=_get_llm(), ) assert "pytest" in result["answer"].lower() @@ -182,6 +225,7 @@ def test_finds_node_test_runner(self, node_express_project): result = run_analyzer( str(node_express_project), "What test runner does this project use?", + llm=_get_llm(), ) assert "jest" in result["answer"].lower() @@ -197,6 +241,7 @@ def test_finds_dockerfile(self, python_flask_project): result = run_analyzer( str(python_flask_project), "Does this project have a Dockerfile? If yes, what base image does it use?", + llm=_get_llm(), ) assert "python" in result["answer"].lower() @@ -204,6 +249,7 @@ def test_finds_npm_scripts(self, node_express_project): result = run_analyzer( str(node_express_project), "What npm scripts are defined in this project?", + llm=_get_llm(), ) assert "start" in result["answer"].lower() assert "test" in result["answer"].lower() @@ -212,6 +258,7 @@ def test_finds_makefile_targets(self, go_project): result = run_analyzer( str(go_project), "What build targets are defined in the Makefile?", + llm=_get_llm(), ) assert "build" in result["answer"].lower() assert "test" in result["answer"].lower() diff --git a/tests/integration/agent/graph/test_hitl_flow.py b/tests/integration/agent/graph/test_hitl_flow.py index 5b74a8f..f6ee813 100644 --- a/tests/integration/agent/graph/test_hitl_flow.py +++ b/tests/integration/agent/graph/test_hitl_flow.py @@ -33,6 +33,39 @@ ) +def _get_llm_config() -> dict: + """Return LLM config based on available API keys.""" + if os.environ.get("GROQ_API_KEY"): + return { + "provider": "groq", + "model": "llama-3.3-70b-versatile", + "api_key": os.environ["GROQ_API_KEY"], + "temperature": 0, + } + elif os.environ.get("OPENAI_API_KEY"): + return { + "provider": "openai", + "model": "gpt-4o", + "api_key": os.environ["OPENAI_API_KEY"], + "temperature": 0, + } + elif os.environ.get("GOOGLE_API_KEY"): + return { + "provider": "gemini", + "model": "gemini-1.5-pro", + "api_key": os.environ["GOOGLE_API_KEY"], + "temperature": 0, + } + elif os.environ.get("ANTHROPIC_API_KEY"): + return { + "provider": "anthropic", + "model": "claude-3-5-sonnet-20241022", + "api_key": os.environ["ANTHROPIC_API_KEY"], + "temperature": 0, + } + raise ValueError("No API key available") + + @pytest.fixture(autouse=True) def _rate_limit_delay(): """Sleep before each test to reduce transient LLM rate-limit failures.""" @@ -118,6 +151,8 @@ async def test_hitl_permission_pause_and_resume_with_real_llm(sample_project): "project_path": str(sample_project), "prompt": prompt, "target_platform": "github_actions", + "thread_id": f"test-{uuid4().hex[:8]}", + "llm": _get_llm_config(), } ) assert stream_response.status_code == 200 @@ -162,7 +197,10 @@ async def test_hitl_permission_pause_and_resume_with_real_llm(sample_project): stream_response = await client.post( f"/generate/{thread_id}/stream/resume", - json={"response": user_reply} + json={ + "response": user_reply, + "llm": _get_llm_config(), + } ) assert stream_response.status_code == 200 @@ -194,7 +232,12 @@ async def test_hitl_permission_pause_and_resume_with_real_llm(sample_project): def test_hitl_normal_clarification_pause_and_resume_real_graph(sample_project): graph = build_graph() thread_id = f"it-clarification-{uuid4()}" - config = {"configurable": {"thread_id": thread_id}} + config = { + "configurable": { + "thread_id": thread_id, + "llm_config": _get_llm_config(), + } + } initial_state = { "messages": [HumanMessage(content="Generate a GitHub Actions workflow")], diff --git a/tests/unit/agent/graph/subgraphs/test_analyzer.py b/tests/unit/agent/graph/subgraphs/test_analyzer.py index 8ed3fb1..888b165 100644 --- a/tests/unit/agent/graph/subgraphs/test_analyzer.py +++ b/tests/unit/agent/graph/subgraphs/test_analyzer.py @@ -15,8 +15,22 @@ def _state(**overrides) -> dict: return base +def _config() -> dict: + """Return a minimal config with llm_config for testing.""" + return { + "configurable": { + "llm_config": { + "provider": "openai", + "model": "gpt-4o", + "api_key": "test-key", + "temperature": 0, + } + } + } + + def test_returns_empty_when_no_query(): - result = analyzer_module.analyzer_node(_state(analyzer_query="")) + result = analyzer_module.analyzer_node(_state(analyzer_query=""), _config()) assert result == {} @@ -36,7 +50,7 @@ def fake_run_analyzer(**kwargs): monkeypatch.setattr(analyzer_module, "run_analyzer", fake_run_analyzer) - result = analyzer_module.analyzer_node(_state()) + result = analyzer_module.analyzer_node(_state(), _config()) assert result["pending_question"]["type"] == "permission" assert result["pending_question"]["source"] == "analyzer" @@ -56,7 +70,7 @@ def test_denied_permission_clears_query_and_records_result(): ], ) - result = analyzer_module.analyzer_node(state) + result = analyzer_module.analyzer_node(state, _config()) assert result["analyzer_query"] == "" assert result["pending_permission_request"] == {} @@ -87,7 +101,7 @@ def fake_run_analyzer(**kwargs): ], ) - result = analyzer_module.analyzer_node(state) + result = analyzer_module.analyzer_node(state, _config()) assert captured["approved_commands"] == {"mkdir build"} assert result["analyzer_query"] == "" @@ -118,7 +132,7 @@ def fake_run_analyzer(**kwargs): ], ) - result = analyzer_module.analyzer_node(state) + result = analyzer_module.analyzer_node(state, _config()) assert "approved_commands" not in captured assert result["analyzer_query"] == "" @@ -139,7 +153,7 @@ def fake_run_analyzer(**kwargs): ], ) - result = analyzer_module.analyzer_node(state) + result = analyzer_module.analyzer_node(state, _config()) assert result["pending_question"]["type"] == "permission" assert "allow' or 'deny'" in result["pending_question"]["message"] diff --git a/tests/unit/agent/llm/__init__.py b/tests/unit/agent/llm/__init__.py new file mode 100644 index 0000000..a1ab707 --- /dev/null +++ b/tests/unit/agent/llm/__init__.py @@ -0,0 +1 @@ +"""Unit tests for LLM module.""" diff --git a/tests/unit/agent/llm/test_factory.py b/tests/unit/agent/llm/test_factory.py new file mode 100644 index 0000000..6762353 --- /dev/null +++ b/tests/unit/agent/llm/test_factory.py @@ -0,0 +1,181 @@ +"""Unit tests for LLM factory.""" + +from unittest.mock import MagicMock, patch + +import pytest + +from agent.api.schemas.streaming import LLMConfig +from agent.llm.factory import build_llm, _litellm_model, _make_hashable_config + + +class TestLLMConfigValidation: + """Test LLMConfig validation rules.""" + + def test_openai_compatible_requires_base_url(self): + """openai_compatible provider must have base_url.""" + with pytest.raises(ValueError, match="requires base_url"): + LLMConfig( + provider="openai_compatible", + model="llama-3.3-70b", + api_key="test-key", + ) + + def test_openai_compatible_with_base_url_passes(self): + """openai_compatible with base_url is valid.""" + config = LLMConfig( + provider="openai_compatible", + model="llama-3.3-70b", + base_url="http://localhost:11434/v1", + ) + assert config.provider == "openai_compatible" + assert config.base_url == "http://localhost:11434/v1" + + def test_first_class_providers_require_api_key(self): + """openai, anthropic, gemini, groq require api_key.""" + for provider in ["openai", "anthropic", "gemini", "groq"]: + with pytest.raises(ValueError, match="requires api_key"): + LLMConfig(provider=provider, model="test-model") # type: ignore + + def test_first_class_providers_with_api_key_pass(self): + """Valid configs for first-class providers.""" + for provider in ["openai", "anthropic", "gemini", "groq"]: + config = LLMConfig( + provider=provider, # type: ignore + model="test-model", + api_key="test-key", + ) + assert config.provider == provider + assert config.api_key == "test-key" + + def test_default_temperature_is_zero(self): + """Default temperature should be 0.""" + config = LLMConfig( + provider="openai", + model="gpt-4o", + api_key="test-key", + ) + assert config.temperature == 0 + + +class TestBuildLLM: + """Test LLM construction.""" + + @patch("agent.llm.factory.ChatLiteLLM") + def test_builds_with_model_and_temperature(self, mock_chat_litellm): + """build_llm passes model and temperature to ChatLiteLLM.""" + mock_instance = MagicMock() + mock_chat_litellm.return_value = mock_instance + + config = LLMConfig( + provider="openai", + model="gpt-4o", + api_key="sk-test", + temperature=0.5, + ) + result = build_llm(config) + + assert result == mock_instance + mock_chat_litellm.assert_called_once_with( + model="openai/gpt-4o", + temperature=0.5, + api_key="sk-test", + ) + + @patch("agent.llm.factory.ChatLiteLLM") + def test_forwards_base_url_as_api_base(self, mock_chat_litellm): + """build_llm forwards base_url to LiteLLM as api_base.""" + mock_instance = MagicMock() + mock_chat_litellm.return_value = mock_instance + + config = LLMConfig( + provider="openai_compatible", + model="llama-3.3-70b", + base_url="http://localhost:11434/v1", + ) + result = build_llm(config) + + assert result == mock_instance + mock_chat_litellm.assert_called_once_with( + model="openai/llama-3.3-70b", + temperature=0, + api_base="http://localhost:11434/v1", + ) + + @patch("agent.llm.factory.ChatLiteLLM") + def test_omits_api_key_when_none(self, mock_chat_litellm): + """build_llm omits api_key if None.""" + mock_instance = MagicMock() + mock_chat_litellm.return_value = mock_instance + + config = LLMConfig( + provider="openai_compatible", + model="llama-3.3-70b-unique", # Use unique model to avoid cache collision + base_url="http://localhost:11434/v1", + api_key=None, + ) + result = build_llm(config) + + # Result should be a mock instance + assert result is not None + mock_chat_litellm.assert_called_once() + call_kwargs = mock_chat_litellm.call_args[1] + assert "api_key" not in call_kwargs + + @patch("agent.llm.factory.ChatLiteLLM") + def test_caching_returns_same_instance(self, mock_chat_litellm): + """build_llm caches and returns the same instance for identical configs.""" + mock_instance = MagicMock() + mock_chat_litellm.return_value = mock_instance + + config1 = LLMConfig(provider="openai", model="gpt-4o", api_key="sk-test") + config2 = LLMConfig(provider="openai", model="gpt-4o", api_key="sk-test") + + result1 = build_llm(config1) + result2 = build_llm(config2) + + # Should return the same cached instance + assert result1 is result2 + # ChatLiteLLM should only be called once + assert mock_chat_litellm.call_count == 1 + + @patch("agent.llm.factory.ChatLiteLLM") + def test_caching_creates_new_instance_for_different_config(self, mock_chat_litellm): + """build_llm creates a new instance for different configs.""" + mock_instance1 = MagicMock() + mock_instance2 = MagicMock() + mock_chat_litellm.side_effect = [mock_instance1, mock_instance2] + + config1 = LLMConfig(provider="openai", model="gpt-4o", api_key="sk-test1") + config2 = LLMConfig(provider="openai", model="gpt-4o", api_key="sk-test2") + + result1 = build_llm(config1) + result2 = build_llm(config2) + + # Should return different instances + assert result1 is not result2 + # ChatLiteLLM should be called twice + assert mock_chat_litellm.call_count == 2 + + +class TestHashableConfig: + """Test config hashing for cache key.""" + + def test_same_config_produces_same_hash(self): + config1 = LLMConfig(provider="openai", model="gpt-4o", api_key="key") + config2 = LLMConfig(provider="openai", model="gpt-4o", api_key="key") + assert _make_hashable_config(config1) == _make_hashable_config(config2) + + def test_different_api_key_produces_different_hash(self): + config1 = LLMConfig(provider="openai", model="gpt-4o", api_key="key1") + config2 = LLMConfig(provider="openai", model="gpt-4o", api_key="key2") + assert _make_hashable_config(config1) != _make_hashable_config(config2) + + def test_none_api_key_handled_correctly(self): + config = LLMConfig( + provider="openai_compatible", + model="llama", + base_url="http://localhost:11434/v1", + api_key=None, + ) + hash_tuple = _make_hashable_config(config) + assert hash_tuple[2] == "" # None converted to empty string From e93826ae89679e36939bb9984ebbdd881f3591e4 Mon Sep 17 00:00:00 2001 From: Hemdan47 Date: Tue, 23 Jun 2026 02:09:29 +0300 Subject: [PATCH 5/8] fix: (tools) replace hardcoded skip-lists with .gitignore-based filtering via pathspec Bug: grep/glob/list tools had inconsistent hardcoded skip directories (e.g., .pytest-cache hyphen vs .pytest_cache underscore in list). Glob had no skip logic at all, walking into .git and .pytest-tmp unnecessarily. Fix: shared _ignore.py module using pathspec.GitIgnoreSpec to honor .gitignore semantics. Only .git is hardcoded because git never lists itself. --- pyproject.toml | 1 + src/agent/tools/glob_tool/glob.py | 7 +- src/agent/tools/grep_tool/grep.py | 23 ++--- src/agent/tools/ignore_filter.py | 89 ++++++++++++++++++++ src/agent/tools/list_tool/list.py | 46 ++++------ tests/unit/agent/tools/test_glob_tool.py | 38 ++++++++- tests/unit/agent/tools/test_grep_tool.py | 32 +++++++ tests/unit/agent/tools/test_ignore_filter.py | 82 ++++++++++++++++++ tests/unit/agent/tools/test_list_tool.py | 34 +++++++- 9 files changed, 303 insertions(+), 49 deletions(-) create mode 100644 src/agent/tools/ignore_filter.py create mode 100644 tests/unit/agent/tools/test_ignore_filter.py diff --git a/pyproject.toml b/pyproject.toml index 8eebd9a..06354f4 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -18,6 +18,7 @@ dependencies = [ "requests>=2.31.0", "html2text>=2020.1.16", "detect-secrets>=1.5,<2", + "pathspec>=0.12.1", # Data & Config "pydantic>=2.0.0", diff --git a/src/agent/tools/glob_tool/glob.py b/src/agent/tools/glob_tool/glob.py index 5c0b9f3..26c30d8 100644 --- a/src/agent/tools/glob_tool/glob.py +++ b/src/agent/tools/glob_tool/glob.py @@ -8,6 +8,7 @@ from pydantic import BaseModel, Field from agent.streaming.tool_run import tool_run +from agent.tools.ignore_filter import IgnoreFilter DESCRIPTION = """Search files matching a glob pattern in a directory. @@ -67,11 +68,13 @@ def _run( ) as ctx: # Use os.walk for recursive ** patterns (more reliable on Windows) matches = [] + ignores = IgnoreFilter(search) if pattern.startswith("**/"): # Pattern like "**/package.json" - search recursively for filename filename = pattern[3:] # Remove "**/ for root, dirs, files in os.walk(str(search)): + dirs[:] = ignores.prune_dirs(root, dirs) if filename in files: matches.append(os.path.join(root, filename)) elif "**" in pattern: @@ -86,9 +89,11 @@ def _run( search_pattern = str(search / pattern) matches = glob.glob(search_pattern, recursive=False) - # collect matches and their modification time + # Drop anything ignored by .gitignore, then collect mtime. files = [] for file in matches: + if ignores.is_ignored(Path(file)): + continue stat = os.stat(file) mtime = stat.st_mtime files.append({"path": str(Path(file).resolve()), "mtime": mtime}) diff --git a/src/agent/tools/grep_tool/grep.py b/src/agent/tools/grep_tool/grep.py index 25d4560..5a050b6 100644 --- a/src/agent/tools/grep_tool/grep.py +++ b/src/agent/tools/grep_tool/grep.py @@ -9,14 +9,10 @@ from pydantic import BaseModel, Field from agent.streaming.tool_run import tool_run +from agent.tools.ignore_filter import IgnoreFilter -# Directories to always skip -_SKIP_DIRS = { - ".git", "__pycache__", "node_modules", ".venv", "venv", - ".tox", ".mypy_cache", ".pytest_cache", "dist", "build", -} - -# Binary file extensions to skip +# Binary file extensions to skip (legit files that aren't in .gitignore +# but should not be grepped as text). _BINARY_EXT = { ".pyc", ".pyo", ".so", ".dll", ".exe", ".bin", ".dat", ".png", ".jpg", ".jpeg", ".gif", ".ico", ".svg", ".bmp", @@ -100,17 +96,22 @@ def _run( matches = [] found_enough = False + ignores = IgnoreFilter(search_path) for root, dirs, files in os.walk(str(search_path)): - # Skip common non-source directories - dirs[:] = [d for d in dirs if d not in _SKIP_DIRS] + # Prune directories ignored by .gitignore (and always-skip .git) + dirs[:] = ignores.prune_dirs(root, dirs) for filename in files: + filepath = os.path.join(root, filename) + + # Skip files ignored by .gitignore + if ignores.is_ignored(Path(filepath)): + continue + # Apply include filter if include and not fnmatch.fnmatch(filename, include): continue - filepath = os.path.join(root, filename) - # Skip binary files if _is_binary(filepath): continue diff --git a/src/agent/tools/ignore_filter.py b/src/agent/tools/ignore_filter.py new file mode 100644 index 0000000..3a970ae --- /dev/null +++ b/src/agent/tools/ignore_filter.py @@ -0,0 +1,89 @@ +"""Shared ignore rules for file-walking tools (grep, glob, list). + +Honors ``.gitignore`` semantics via ``pathspec``. Git never lists itself in a +``.gitignore``, so ``.git`` is hardcoded as always-skip. + +Usage:: + + ig = IgnoreFilter(root) + for root, dirs, files in os.walk(root): + dirs[:] = ig.prune_dirs(root, dirs) + for f in files: + if ig.is_ignored(Path(root) / f): + continue +""" +from __future__ import annotations + +import os +from pathlib import Path +from typing import List, Tuple + +import pathspec + +# git never lists itself in .gitignore; this is the only principled hardcode. +_ALWAYS_SKIP_DIRS = {".git"} + + +class IgnoreFilter: + """Evaluates .gitignore rules for a search root. + + All ``.gitignore`` files found under ``root`` are loaded once and matched + relative to their own directory, so nested gitignores and negations (``!``) + behave exactly like real git. + """ + + def __init__(self, root: Path) -> None: + self._root = Path(root).resolve() + # list of (gitignore_dir, spec): patterns apply relative to gitignore_dir. + self._specs: List[Tuple[Path, pathspec.GitIgnoreSpec]] = self._load(self._root) + + @staticmethod + def _load(root: Path) -> List[Tuple[Path, pathspec.GitIgnoreSpec]]: + specs: List[Tuple[Path, pathspec.GitIgnoreSpec]] = [] + for dirpath, _dirnames, filenames in os.walk(str(root)): + if ".gitignore" not in filenames: + continue + gi_path = Path(dirpath) / ".gitignore" + try: + lines = gi_path.read_text(encoding="utf-8", errors="ignore").splitlines() + except OSError: + continue + specs.append((Path(dirpath), pathspec.GitIgnoreSpec.from_lines(lines))) + return specs + + def is_ignored(self, path: Path, is_dir: bool = False) -> bool: + """True if ``path`` is ignored by any applicable .gitignore.""" + ignored = False + for spec_dir, spec in self._specs: + rel = self._relative_to(path, spec_dir) + if rel is None: + continue + if is_dir and spec.match_file(rel + "/"): + ignored = True + elif spec.match_file(rel): + ignored = True + return ignored + + @staticmethod + def _relative_to(path: Path, base: Path): + try: + p = path.resolve() if not path.is_absolute() else path + rel = Path(os.path.relpath(str(p), str(base))) + except (ValueError, OSError): + return None + rel_str = str(rel) + if rel_str == "." or rel_str.startswith(".."): + return None + return rel_str.replace(os.sep, "/") + + def is_skipped_dir(self, dirpath: Path) -> bool: + """True if the directory itself should be pruned during a walk.""" + name = dirpath.name + if name in _ALWAYS_SKIP_DIRS: + return True + return self.is_ignored(dirpath, is_dir=True) + + def prune_dirs(self, current_root: str, dirs: List[str]) -> List[str]: + """Return the subset of ``dirs`` (in ``current_root``) that are kept.""" + base = Path(current_root) + return [d for d in dirs if not self.is_skipped_dir(base / d)] diff --git a/src/agent/tools/list_tool/list.py b/src/agent/tools/list_tool/list.py index 6c5f692..70f63e7 100644 --- a/src/agent/tools/list_tool/list.py +++ b/src/agent/tools/list_tool/list.py @@ -8,33 +8,7 @@ from pydantic import BaseModel, Field from agent.streaming.tool_run import tool_run - - -IGNORE_PATTERNS = [ - "node_modules/", - "__pycache__/", - ".git/", - "dist/", - "build/", - "target/", - "vendor/", - "bin/", - "obj/", - ".idea/", - ".vscode/", - ".zig-cache/", - "zig-out", - ".coverage", - "coverage/", - "tmp/", - "temp/", - ".cache/", - "cache/", - "logs/", - ".venv/", - "venv/", - "env/", -] +from agent.tools.ignore_filter import IgnoreFilter DESCRIPTION = """List files and directories in a tree-like structure, with ignore rules. @@ -93,15 +67,25 @@ def _run( # Use provided path or fall back to working_dir resolved = Path(path if path else self.working_dir).resolve() with tool_run("list", f"Listing {resolved.name}", {"path": str(resolved)}) as ctx: - ignore_patterns = IGNORE_PATTERNS + (ignore if ignore else []) + ignores = IgnoreFilter(resolved) + extra_ignore = ignore or [] files = [] for root, dirs, filenames in os.walk(resolved): - dirs[:] = [d for d in dirs if not any(fnmatch.fnmatch(d, pat.strip("/")) for pat in ignore_patterns)] + # Prune ignored dirs: .gitignore rules + user extra ignore globs. + dirs[:] = [ + d for d in ignores.prune_dirs(root, dirs) + if not any(fnmatch.fnmatch(d, pat.strip("/")) for pat in extra_ignore) + ] for filename in filenames: - rel_path = str(Path(root).relative_to(resolved) / filename) - if any(fnmatch.fnmatch(rel_path, pat.strip("/")) for pat in ignore_patterns): + filepath = Path(root) / filename + # Skip files ignored by .gitignore. + if ignores.is_ignored(filepath): + continue + rel_path = str(filepath.relative_to(resolved)) + # Skip files matching the user's extra ignore globs. + if any(fnmatch.fnmatch(rel_path, pat.strip("/")) for pat in extra_ignore): continue files.append(rel_path) diff --git a/tests/unit/agent/tools/test_glob_tool.py b/tests/unit/agent/tools/test_glob_tool.py index d791c63..753041a 100644 --- a/tests/unit/agent/tools/test_glob_tool.py +++ b/tests/unit/agent/tools/test_glob_tool.py @@ -91,17 +91,49 @@ def test_explicit_path_overrides_working_directory(self, tmp_path): project_dir = tmp_path / "project" project_dir.mkdir() (project_dir / "project_file.py").write_text("code") - + other_dir = tmp_path / "other" other_dir.mkdir() (other_dir / "other_file.py").write_text("code") # Create tool with project_dir as working_directory tool = create_glob_tool(working_directory=str(project_dir)) - + # Call with explicit path to other_dir - should search other_dir result = tool.invoke({"pattern": "*.py", "path": str(other_dir)}) assert result["metadata"]["count"] == 1 assert "other_file.py" in result["output"] - assert "project_file.py" not in result["output"] \ No newline at end of file + assert "project_file.py" not in result["output"] + + def test_skips_gitignored_files(self, tmp_path): + (tmp_path / ".gitignore").write_text("node_modules/\n*.log\n") + nm = tmp_path / "node_modules" + nm.mkdir() + (nm / "pkg.py").write_text("code") + (tmp_path / "trace.log").write_text("x") + (tmp_path / "app.py").write_text("code") + + glob_tool = create_glob_tool() + result = glob_tool.invoke({"pattern": "*.py", "path": str(tmp_path)}) + + assert result["metadata"]["count"] == 1 + assert "app.py" in result["output"] + assert "pkg.py" not in result["output"] + assert "trace.log" not in result["output"] + + def test_recursive_glob_skips_ignored_dirs(self, tmp_path): + (tmp_path / ".gitignore").write_text("build/\n") + build = tmp_path / "build" + build.mkdir() + (build / "target.py").write_text("code") + src = tmp_path / "src" + src.mkdir() + (src / "target.py").write_text("code") + + glob_tool = create_glob_tool() + result = glob_tool.invoke({"pattern": "**/target.py", "path": str(tmp_path)}) + + assert result["metadata"]["count"] == 1 + assert "src" in result["output"] + assert "build" not in result["output"] \ No newline at end of file diff --git a/tests/unit/agent/tools/test_grep_tool.py b/tests/unit/agent/tools/test_grep_tool.py index fcb8b9f..c42fcc4 100644 --- a/tests/unit/agent/tools/test_grep_tool.py +++ b/tests/unit/agent/tools/test_grep_tool.py @@ -70,6 +70,38 @@ def test_skips_binary_files(self, tmp_path): assert "code.py" in result["output"] assert "image.png" not in result["output"] + def test_skips_gitignored_files(self, tmp_path): + # A .gitignore'd file must not be grepped, even though it's not binary + # and would otherwise match. + (tmp_path / ".gitignore").write_text("secrets/\n*.log\n") + secret_dir = tmp_path / "secrets" + secret_dir.mkdir() + (secret_dir / "env.txt").write_text("findme here") + (tmp_path / "trace.log").write_text("findme here") + (tmp_path / "app.py").write_text("findme here") + + grep_tool = create_grep_tool() + result = grep_tool.invoke({"pattern": "findme", "path": str(tmp_path)}) + + assert "app.py" in result["output"] + assert "trace.log" not in result["output"] + assert "secrets" not in result["output"] + assert "env.txt" not in result["output"] + + def test_skips_dot_git_directory(self, tmp_path): + # .git is never in .gitignore but must always be skipped. + git_dir = tmp_path / ".git" + git_dir.mkdir() + (git_dir / "config").write_text("findme here") + (tmp_path / "app.py").write_text("findme here") + + grep_tool = create_grep_tool() + result = grep_tool.invoke({"pattern": "findme", "path": str(tmp_path)}) + + assert "app.py" in result["output"] + assert ".git" not in result["output"] + assert result["metadata"]["count"] == 1 + def test_uses_working_directory_when_path_not_provided(self, tmp_path): """Test that the tool uses configured working_directory when path is omitted.""" (tmp_path / "project_file.py").write_text("searchterm here") diff --git a/tests/unit/agent/tools/test_ignore_filter.py b/tests/unit/agent/tools/test_ignore_filter.py new file mode 100644 index 0000000..c7bf8c5 --- /dev/null +++ b/tests/unit/agent/tools/test_ignore_filter.py @@ -0,0 +1,82 @@ +"""Unit tests for the shared ignore filter (agent.tools._ignore).""" +from pathlib import Path + +from agent.tools.ignore_filter import IgnoreFilter + + +class TestIgnoreFilter: + def test_gitignore_skips_file(self, tmp_path): + (tmp_path / ".gitignore").write_text("*.log\n") + (tmp_path / "app.log").write_text("x") + (tmp_path / "app.py").write_text("x") + + ig = IgnoreFilter(tmp_path) + assert ig.is_ignored(tmp_path / "app.log") is True + assert ig.is_ignored(tmp_path / "app.py") is False + + def test_directory_pattern_prunes_dir(self, tmp_path): + (tmp_path / ".gitignore").write_text("node_modules/\n") + nm = tmp_path / "node_modules" + nm.mkdir() + (nm / "pkg.js").write_text("x") + src = tmp_path / "src" + src.mkdir() + + ig = IgnoreFilter(tmp_path) + assert ig.is_skipped_dir(nm) is True + assert ig.is_skipped_dir(src) is False + + def test_git_always_skipped_without_gitignore_entry(self, tmp_path): + # .git is never listed in .gitignore but must still be skipped. + (tmp_path / ".gitignore").write_text("*.log\n") + git_dir = tmp_path / ".git" + git_dir.mkdir() + + ig = IgnoreFilter(tmp_path) + assert ig.is_skipped_dir(git_dir) is True + + def test_negation_reincludes_file(self, tmp_path): + # !keep.log should re-include despite *.log + (tmp_path / ".gitignore").write_text("*.log\n!keep.log\n") + + ig = IgnoreFilter(tmp_path) + assert ig.is_ignored(tmp_path / "drop.log") is True + assert ig.is_ignored(tmp_path / "keep.log") is False + + def test_nested_gitignore_applies_relative_to_its_dir(self, tmp_path): + (tmp_path / ".gitignore").write_text("*.log\n") + sub = tmp_path / "sub" + sub.mkdir() + (sub / ".gitignore").write_text("*.tmp\n!keep.tmp\n") + + ig = IgnoreFilter(tmp_path) + # root rule applies at root + assert ig.is_ignored(tmp_path / "app.log") is True + # nested rule applies only under sub/, and negation is honored + assert ig.is_ignored(sub / "data.tmp") is True + assert ig.is_ignored(sub / "keep.tmp") is False + # root *.log should NOT ignore a .tmp under sub + assert ig.is_ignored(sub / "data.tmp") is True + # a .tmp at root is not ignored (nested rule scope is sub/ only) + assert ig.is_ignored(tmp_path / "root.tmp") is False + + def test_prune_dirs_keeps_unignored(self, tmp_path): + (tmp_path / ".gitignore").write_text("build/\n") + (tmp_path / "src").mkdir() + (tmp_path / "build").mkdir() + (tmp_path / ".git").mkdir() + + ig = IgnoreFilter(tmp_path) + kept = ig.prune_dirs(str(tmp_path), ["src", "build", ".git", "tests"]) + assert "src" in kept + assert "tests" in kept + assert "build" not in kept + assert ".git" not in kept + + def test_no_gitignore_ignores_nothing_except_dotgit(self, tmp_path): + (tmp_path / "app.py").write_text("x") + ig = IgnoreFilter(tmp_path) + assert ig.is_ignored(tmp_path / "app.py") is False + git_dir = tmp_path / ".git" + git_dir.mkdir() + assert ig.is_skipped_dir(git_dir) is True diff --git a/tests/unit/agent/tools/test_list_tool.py b/tests/unit/agent/tools/test_list_tool.py index e8357e3..3507ce2 100644 --- a/tests/unit/agent/tools/test_list_tool.py +++ b/tests/unit/agent/tools/test_list_tool.py @@ -84,17 +84,45 @@ def test_explicit_path_overrides_working_directory(self, tmp_path): project_dir = tmp_path / "project" project_dir.mkdir() (project_dir / "project_file.py").write_text("code") - + other_dir = tmp_path / "other" other_dir.mkdir() (other_dir / "other_file.py").write_text("code") # Create tool with project_dir as working_directory tool = create_list_tool(working_directory=str(project_dir)) - + # Call with explicit path to other_dir - should list other_dir result = tool.invoke({"path": str(other_dir)}) assert result["metadata"]["count"] == 1 assert "other_file.py" in result["output"] - assert "project_file.py" not in result["output"] \ No newline at end of file + assert "project_file.py" not in result["output"] + + def test_skips_gitignored_entries(self, tmp_path): + (tmp_path / ".gitignore").write_text("node_modules/\n*.log\n") + nm = tmp_path / "node_modules" + nm.mkdir() + (nm / "pkg.js").write_text("x") + (tmp_path / "trace.log").write_text("x") + (tmp_path / "app.py").write_text("x") + + list_tool = create_list_tool() + result = list_tool.invoke({"path": str(tmp_path)}) + + assert "app.py" in result["output"] + assert "node_modules" not in result["output"] + assert "trace.log" not in result["output"] + + def test_user_ignore_still_works_besides_gitignore(self, tmp_path): + (tmp_path / ".gitignore").write_text("*.log\n") + (tmp_path / "keep.py").write_text("x") + (tmp_path / "skip.md").write_text("x") + (tmp_path / "trace.log").write_text("x") + + list_tool = create_list_tool() + result = list_tool.invoke({"path": str(tmp_path), "ignore": ["*.md"]}) + + assert "keep.py" in result["output"] + assert "skip.md" not in result["output"] + assert "trace.log" not in result["output"] \ No newline at end of file From 8bcf7d7bec5873196b9d688562455bce16a5d940 Mon Sep 17 00:00:00 2001 From: Hemdan47 Date: Wed, 24 Jun 2026 19:26:00 +0300 Subject: [PATCH 6/8] feat: add LLM resilience and targeted exception handling. Make LLM failures self-heal and visible instead of crashing opaquely: two-layer retry (silent LiteLLM num_retries + visible LangChain .with_retry), error classification into actionable categories, and a new "llm.error" event. Also guard I/O boundaries (writer disk failures, glob/grep errors) and catch GraphRecursionError as a "stuck in loop" event. Fixes two runtime import bugs and adds unit tests for the new code. --- src/agent/api/routes/streaming.py | 37 +++- src/agent/exceptions/__init__.py | 8 +- src/agent/exceptions/config.py | 14 ++ src/agent/exceptions/llm.py | 48 +++++ src/agent/graph/nodes/planner.py | 25 ++- src/agent/graph/nodes/writer.py | 36 +++- src/agent/graph/subgraphs/analyzer.py | 6 + .../generator_reflexion/generator.py | 25 ++- src/agent/llm/factory.py | 50 +++-- src/agent/llm/resilience.py | 163 ++++++++++++++ src/agent/streaming/emitter.py | 42 ++++ src/agent/streaming/events.py | 1 + src/agent/streaming/runner.py | 14 ++ src/agent/tools/glob_tool/glob.py | 10 +- src/agent/tools/grep_tool/grep.py | 10 +- src/agent/tools/ignore_filter.py | 7 +- tests/unit/agent/exceptions/__init__.py | 0 tests/unit/agent/graph/nodes/test_writer.py | 45 +++- .../agent/graph/subgraphs/test_analyzer.py | 21 ++ tests/unit/agent/llm/test_factory.py | 27 +++ tests/unit/agent/llm/test_resilience.py | 202 ++++++++++++++++++ tests/unit/agent/streaming/test_events.py | 1 + tests/unit/agent/streaming/test_runner.py | 49 ++++- tests/unit/agent/tools/test_glob_tool.py | 37 +++- tests/unit/agent/tools/test_grep_tool.py | 20 +- 25 files changed, 844 insertions(+), 54 deletions(-) create mode 100644 src/agent/exceptions/config.py create mode 100644 src/agent/exceptions/llm.py create mode 100644 src/agent/llm/resilience.py create mode 100644 tests/unit/agent/exceptions/__init__.py create mode 100644 tests/unit/agent/llm/test_resilience.py diff --git a/src/agent/api/routes/streaming.py b/src/agent/api/routes/streaming.py index 043a9f1..26634a7 100644 --- a/src/agent/api/routes/streaming.py +++ b/src/agent/api/routes/streaming.py @@ -5,12 +5,12 @@ from datetime import datetime, timezone from uuid import uuid4 -from fastapi import APIRouter +from fastapi import APIRouter, HTTPException from fastapi.responses import StreamingResponse from langgraph.types import Command from agent.api.schemas.streaming import ResumeRequest, StreamRequest, build_messages -from agent.streaming.runner import request_cancel, stream_graph +from agent.streaming.runner import graph, request_cancel, stream_graph router = APIRouter(tags=["Generation"]) @@ -74,15 +74,30 @@ async def generate_stream(request: StreamRequest): @router.post("/generate/{thread_id}/stream/resume") async def resume_stream(thread_id: str, request: ResumeRequest): - """Resume an interrupted workflow generation stream.""" - run_id = f"run_{uuid4().hex[:12]}" + """Resume an interrupted workflow generation stream. + + Raises: + HTTPException: 404 if thread_id does not exist + """ + # Validate thread exists before attempting resume config = { "configurable": { "thread_id": thread_id, "llm_config": request.llm.model_dump(), } } - + + state = graph.get_state(config) + + # MemorySaver returns empty state for unknown threads + if not state.values: + raise HTTPException( + status_code=404, + detail=f"Thread '{thread_id}' not found. Cannot resume non-existent thread.", + ) + + run_id = f"run_{uuid4().hex[:12]}" + return StreamingResponse( stream_graph( Command(resume=request.response), @@ -103,7 +118,15 @@ async def resume_stream(thread_id: str, request: ResumeRequest): @router.post("/generate/runs/{run_id}/cancel") async def cancel_generation(run_id: str): - """Signal a running generation stream to stop.""" + """Signal a running generation stream to stop. + + Raises: + HTTPException: 404 if run_id does not exist + """ if request_cancel(run_id): return {"status": "cancelling", "run_id": run_id} - return {"status": "not_found", "run_id": run_id} + + raise HTTPException( + status_code=404, + detail=f"Run '{run_id}' not found or already completed.", + ) diff --git a/src/agent/exceptions/__init__.py b/src/agent/exceptions/__init__.py index 608e1a2..96d9a31 100644 --- a/src/agent/exceptions/__init__.py +++ b/src/agent/exceptions/__init__.py @@ -1,5 +1,11 @@ """Package initialization for agent-wide exceptions.""" +from .config import AgentConfigError +from .llm import LLMInvocationError from .tools import PermissionRequiredException -__all__ = ["PermissionRequiredException"] +__all__ = [ + "AgentConfigError", + "LLMInvocationError", + "PermissionRequiredException", +] diff --git a/src/agent/exceptions/config.py b/src/agent/exceptions/config.py new file mode 100644 index 0000000..1de53b7 --- /dev/null +++ b/src/agent/exceptions/config.py @@ -0,0 +1,14 @@ +"""Configuration-related exceptions for the agent.""" + + +class AgentConfigError(ValueError): + """Raised when agent configuration is invalid or incomplete. + + Examples: + - Unknown LLM provider + - Missing required configuration fields + - Invalid model names + - Malformed configuration structure + + This is a caller-fixable error that should fail fast with a clear message. + """ diff --git a/src/agent/exceptions/llm.py b/src/agent/exceptions/llm.py new file mode 100644 index 0000000..860a71d --- /dev/null +++ b/src/agent/exceptions/llm.py @@ -0,0 +1,48 @@ +"""LLM invocation exceptions with actionable classification.""" + +from __future__ import annotations + + +class LLMInvocationError(Exception): + """LLM/LangChain invocation failure with actionable classification. + + This exception wraps provider-specific errors (LiteLLM, OpenAI, Anthropic, etc.) + into a consistent, actionable format for error handling and user notification. + + Attributes: + provider: The LLM provider (e.g., "openai", "anthropic", "groq") + reason: Human-readable error description + severity: "transient" (retryable) or "fatal" (do not retry) + category: Error category for user notification + - "rate_limit": 429 Too Many Requests + - "timeout": Request timeout + - "unavailable": 5xx Server errors + - "parse": Structured output schema mismatch + - "auth": 401 Authentication failed + - "bad_request": 400 Malformed request + - "unknown": Unclassified error + status_code: HTTP status code if applicable + attempts: Number of attempts made before giving up + """ + + def __init__( + self, + provider: str, + reason: str, + *, + severity: str, + category: str, + status_code: int | None = None, + attempts: int = 0, + ): + self.provider = provider + self.reason = reason + self.severity = severity + self.category = category + self.status_code = status_code + self.attempts = attempts + + super().__init__( + f"[{provider}] {category} ({severity}): {reason}" + + (f" after {attempts} attempts" if attempts > 1 else "") + ) diff --git a/src/agent/graph/nodes/planner.py b/src/agent/graph/nodes/planner.py index 76fbb2b..d3fbe6b 100644 --- a/src/agent/graph/nodes/planner.py +++ b/src/agent/graph/nodes/planner.py @@ -17,7 +17,8 @@ from agent.graph.schemas import PlannerOutput, PendingQuestion from agent.graph.state import State from agent.llm.helpers import get_llm_from_config -from agent.streaming.emitter import emit_plan, emit_title, emit_update +from agent.llm.resilience import build_resilient_llm, classify_llm_error +from agent.streaming.emitter import emit_plan, emit_title, emit_update, emit_llm_error def _build_context_block(state: State) -> str: @@ -140,7 +141,7 @@ def planner_node(state: State, config: RunnableConfig) -> dict: } llm = get_llm_from_config(config) - structured_llm = llm.with_structured_output(PlannerOutput, method="function_calling") + structured_llm = build_resilient_llm(llm, PlannerOutput) target_platform = state.get("target_platform", "github_actions") context_block = _build_context_block(state) @@ -154,10 +155,22 @@ def planner_node(state: State, config: RunnableConfig) -> dict: conversation = list(state.get("messages", [])) conversation.append(HumanMessage(content=context_block)) - response: PlannerOutput = structured_llm.invoke([ - SystemMessage(content=system_prompt), - *conversation, - ]) + try: + response: PlannerOutput = structured_llm.invoke([ + SystemMessage(content=system_prompt), + *conversation, + ]) + except Exception as exc: + # Classify and emit error for user notification + severity, category, status_code = classify_llm_error(exc) + emit_llm_error( + category=category, + severity=severity, + message=f"Planning failed: {category}", + status_code=status_code, + ) + # Re-raise to let the runner handle terminal failure + raise if response.public_update: emit_update(response.public_update) diff --git a/src/agent/graph/nodes/writer.py b/src/agent/graph/nodes/writer.py index eb7f567..5565c11 100644 --- a/src/agent/graph/nodes/writer.py +++ b/src/agent/graph/nodes/writer.py @@ -11,9 +11,11 @@ from pathlib import Path from langchain_core.messages import AIMessage +from langgraph.config import get_stream_writer from agent.graph.state import State from agent.streaming.emitter import emit_run_completed +from agent.streaming.events import StreamEvent from agent.tools import write_tool MAX_FILENAME_ATTEMPTS = 100 @@ -42,7 +44,12 @@ def _resolve_output_path(project_path: str, planned_filepath: str) -> str: def write_yaml(project_path: str, yaml_draft: str, planned_filepath: str) -> str: - """Write the YAML draft to disk and return the absolute path.""" + """Write the YAML draft to disk and return the absolute path. + + Raises: + OSError: If the file cannot be written (disk full, permissions, etc.) + RuntimeError: If a unique filename cannot be found + """ file_path = _resolve_output_path(project_path, planned_filepath) write_tool.invoke({"filePath": file_path, "content": yaml_draft}) return file_path @@ -70,11 +77,28 @@ def writer_node(state: State) -> dict: else ".github/workflows/ci.yml" ) - path = write_yaml( - project_path=project_path, - yaml_draft=yaml_draft, - planned_filepath=planned_filepath, - ) + try: + path = write_yaml( + project_path=project_path, + yaml_draft=yaml_draft, + planned_filepath=planned_filepath, + ) + except (OSError, RuntimeError) as e: + # OSError: Disk write failure (permissions, disk full, read-only mount) + # RuntimeError: Filename collision exhausted (extremely rare) + # Emit failure event with draft content so client can offer download + writer = get_stream_writer() + writer(StreamEvent( + type="run.failed", + node="writer", + data={ + "message": f"Failed to write workflow file: {e}", + "detail": type(e).__name__, + "resolved_path": str(Path(project_path) / planned_filepath), + "draft_content": yaml_draft, + }, + ).model_dump(mode="json")) + raise emit_run_completed( output_path=path, diff --git a/src/agent/graph/subgraphs/analyzer.py b/src/agent/graph/subgraphs/analyzer.py index 3ca9737..677aa83 100644 --- a/src/agent/graph/subgraphs/analyzer.py +++ b/src/agent/graph/subgraphs/analyzer.py @@ -118,6 +118,12 @@ def run_analyzer( except PermissionRequiredException as e: permission_request = e.payload answer = "Analysis halted: Permission required." + except Exception as exc: + # Catch any other error (LLM failures, tool crashes, etc.) and return + # a degraded answer instead of crashing. The ReAct loop's accumulated + # context is lost, but the planner/generator can continue. + answer = f"Analysis failed: {type(exc).__name__}: {exc}" + permission_request = None return { "answer": answer, diff --git a/src/agent/graph/subgraphs/generator_reflexion/generator.py b/src/agent/graph/subgraphs/generator_reflexion/generator.py index 9c07489..501fcaf 100644 --- a/src/agent/graph/subgraphs/generator_reflexion/generator.py +++ b/src/agent/graph/subgraphs/generator_reflexion/generator.py @@ -19,7 +19,8 @@ from agent.graph.schemas import GeneratorOutput, PendingQuestion from agent.graph.state import State from agent.llm.helpers import get_llm_from_config -from agent.streaming.emitter import emit_update +from agent.llm.resilience import build_resilient_llm, classify_llm_error +from agent.streaming.emitter import emit_update, emit_llm_error from agent.utils import clean_yaml_fences @@ -80,7 +81,7 @@ def generator_node(state: State, config: RunnableConfig) -> dict: Returns a partial state dict — only the fields this node writes. """ llm = get_llm_from_config(config) - structured_llm = llm.with_structured_output(GeneratorOutput, method="function_calling") + structured_llm = build_resilient_llm(llm, GeneratorOutput) target_platform = state.get("target_platform", "github_actions") @@ -95,10 +96,22 @@ def generator_node(state: State, config: RunnableConfig) -> dict: conversation = list(state.get("messages", [])) conversation.append(HumanMessage(content=user_message)) - response: GeneratorOutput = structured_llm.invoke([ - SystemMessage(content=system_prompt), - *conversation, - ]) + try: + response: GeneratorOutput = structured_llm.invoke([ + SystemMessage(content=system_prompt), + *conversation, + ]) + except Exception as exc: + # Classify and emit error for user notification + severity, category, status_code = classify_llm_error(exc) + emit_llm_error( + category=category, + severity=severity, + message=f"Generation failed: {category}", + status_code=status_code, + ) + # Re-raise to let the runner handle terminal failure + raise if response.public_update: emit_update(response.public_update) diff --git a/src/agent/llm/factory.py b/src/agent/llm/factory.py index 33c4e97..c45d9b9 100644 --- a/src/agent/llm/factory.py +++ b/src/agent/llm/factory.py @@ -8,13 +8,12 @@ from __future__ import annotations from functools import lru_cache -from typing import TYPE_CHECKING from langchain_core.language_models import BaseChatModel from langchain_litellm import ChatLiteLLM + from agent.api.schemas.streaming import LLMConfig -if TYPE_CHECKING: - from agent.api.schemas.streaming import LLMConfig +from agent.exceptions import AgentConfigError _PROVIDER_PREFIX = { @@ -27,7 +26,16 @@ def _litellm_model(cfg: "LLMConfig") -> str: - """Convert our provider discriminator to LiteLLM's model string.""" + """Convert our provider discriminator to LiteLLM's model string. + + Raises: + AgentConfigError: If the provider is not supported + """ + if cfg.provider not in _PROVIDER_PREFIX: + supported = ", ".join(sorted(_PROVIDER_PREFIX.keys())) + raise AgentConfigError( + f"Unknown provider '{cfg.provider}'. Supported: {supported}" + ) return f"{_PROVIDER_PREFIX[cfg.provider]}/{cfg.model}" @@ -44,9 +52,12 @@ def _make_hashable_config(cfg: "LLMConfig") -> tuple: @lru_cache(maxsize=32) def _cached_build_llm(cfg_tuple: tuple) -> BaseChatModel: - """Internal cached builder. Do not call directly.""" + """Internal cached builder. Do not call directly. + + Raises: + AgentConfigError: If LLM construction fails due to config issues + """ # Reconstruct a minimal config-like object - provider, model, api_key, base_url, temperature = cfg_tuple cfg = LLMConfig( provider=provider, @@ -56,16 +67,23 @@ def _cached_build_llm(cfg_tuple: tuple) -> BaseChatModel: temperature=temperature, ) - kwargs = { - "model": _litellm_model(cfg), - "temperature": cfg.temperature, - } - if cfg.api_key: - kwargs["api_key"] = cfg.api_key - if cfg.base_url: - kwargs["api_base"] = cfg.base_url # LiteLLM's param is named api_base - - return ChatLiteLLM(**kwargs) + try: + kwargs = { + "model": _litellm_model(cfg), + "temperature": cfg.temperature, + "num_retries": 3, # Layer 1: silent transport-level retry + } + if cfg.api_key: + kwargs["api_key"] = cfg.api_key + if cfg.base_url: + kwargs["api_base"] = cfg.base_url # LiteLLM's param is named api_base + + return ChatLiteLLM(**kwargs) + except Exception as e: + # Construction failures are typically config issues (bad model, auth, etc) + raise AgentConfigError( + f"Failed to build LLM for provider '{cfg.provider}': {e}" + ) from e def build_llm(cfg: "LLMConfig") -> BaseChatModel: diff --git a/src/agent/llm/resilience.py b/src/agent/llm/resilience.py new file mode 100644 index 0000000..b57cc85 --- /dev/null +++ b/src/agent/llm/resilience.py @@ -0,0 +1,163 @@ +"""LLM resilience layer with two-level retry and error classification. + +This module provides: +1. Error classification: Maps provider-specific exceptions to actionable categories +2. Resilient LLM construction: Wraps structured output with retry + notification +3. Two-layer retry strategy: + - Layer 1 (LiteLLM num_retries): Silent transport-level retry + - Layer 2 (LangChain .with_retry()): Visible application-level retry with notification +""" + +from __future__ import annotations + +from typing import Any + +from agent.streaming.emitter import emit_llm_error +from langchain_core.exceptions import OutputParserException +from langchain_core.language_models import BaseChatModel +from litellm.exceptions import ( + APIConnectionError, + AuthenticationError, + BadRequestError, + NotFoundError, + PermissionDeniedError, + RateLimitError, + ServiceUnavailableError, + Timeout, +) +from pydantic import BaseModel +from tenacity import RetryCallState + + +# Retryable exception types for Layer 2 (.with_retry()) +# These include both parse errors and transport errors that survived Layer 1 +_RETRYABLE_EXCEPTIONS = ( + # Parse errors (schema mismatch) + OutputParserException, + ValueError, + # Transport errors (only reach here after num_retries exhausted) + RateLimitError, + Timeout, + ServiceUnavailableError, + APIConnectionError, +) + + +def classify_llm_error(exc: Exception) -> tuple[str, str, int | None]: + """Classify an LLM exception into (severity, category, status_code). + + This runs AFTER both retry layers have been exhausted, so any exception + here represents a sustained or non-retryable failure. + + Args: + exc: The exception raised by the LLM invocation + + Returns: + Tuple of (severity, category, status_code): + - severity: "transient" or "fatal" + - category: "rate_limit", "timeout", "unavailable", "parse", "auth", + "bad_request", or "unknown" + - status_code: HTTP status code if available, else None + + """ + exc_type = type(exc).__name__ + status_code = getattr(exc, "status_code", None) + + # Parse errors (schema mismatch) - transient, may succeed with retry + if isinstance(exc, (OutputParserException, ValueError)): + return ("transient", "parse", None) + + # Rate limiting (429) - transient but exhausted retries + if isinstance(exc, RateLimitError) or status_code == 429: + return ("transient", "rate_limit", 429) + + # Timeout - transient + if isinstance(exc, Timeout) or "timeout" in exc_type.lower(): + return ("transient", "timeout", status_code) + + # Service unavailable (5xx) - transient + if isinstance(exc, (ServiceUnavailableError, APIConnectionError)): + return ("transient", "unavailable", status_code) + if status_code and 500 <= status_code < 600: + return ("transient", "unavailable", status_code) + + # Authentication errors (401, 403) - fatal + if isinstance(exc, (AuthenticationError, PermissionDeniedError)): + return ("fatal", "auth", status_code or 401) + if status_code in (401, 403): + return ("fatal", "auth", status_code) + + # Bad request (400, 404) - fatal + if isinstance(exc, (BadRequestError, NotFoundError)): + return ("fatal", "bad_request", status_code or 400) + if status_code in (400, 404): + return ("fatal", "bad_request", status_code) + + # Unknown - fatal (surface as-is) + return ("fatal", "unknown", status_code) + + +def _on_retry_callback(retry_state: RetryCallState) -> None: + """Emit a transient llm.error event on each retry attempt. + + This callback fires during Layer 2 retries (.with_retry()), providing + user notification that a sustained issue is being retried. + """ + + exc = retry_state.outcome.exception() + severity, category, status_code = classify_llm_error(exc) + attempt = retry_state.attempt_number + + emit_llm_error( + category=category, + severity="transient", # Always transient during retry + attempts=attempt, + status_code=status_code, + message=f"{category} — retrying ({attempt})…", + ) + + +def build_resilient_llm( + llm: BaseChatModel, + schema: type[BaseModel], + *, + max_retries: int = 2, +) -> Any: + """Build a structured-output LLM with built-in retry + notification. + + This provides two-layer error handling: + + Layer 1 (LiteLLM num_retries, configured in factory.py): + - Silently retries transport errors (429/5xx/timeout) + - Fast recovery for transient blips + - Respects provider Retry-After headers + - User sees nothing if it succeeds + + Layer 2 (LangChain .with_retry(), this function): + - Catches transport errors that exhausted Layer 1 (sustained issues) + - Also catches parse errors (schema mismatch) + - Emits llm.error events via before_sleep callback + - User sees "retrying (N)…" notification + + On final failure after all retries, the exception propagates to the + call site for classification via classify_llm_error() → LLMInvocationError. + + Args: + llm: Base LangChain chat model (configured with num_retries in factory) + schema: Pydantic model for structured output + max_retries: Maximum retry attempts for Layer 2 (default: 2) + Combined with num_retries=3, max 6 total HTTP attempts + + Returns: + A Runnable that parses into `schema` with built-in resilience + + """ + return ( + llm.with_structured_output(schema, method="function_calling") + .with_retry( + retry_if_exception_type=_RETRYABLE_EXCEPTIONS, + stop_after_attempt=max_retries, + wait_exponential_jitter=True, + before_sleep=_on_retry_callback, + ) + ) diff --git a/src/agent/streaming/emitter.py b/src/agent/streaming/emitter.py index a74c92e..335d9c8 100644 --- a/src/agent/streaming/emitter.py +++ b/src/agent/streaming/emitter.py @@ -98,3 +98,45 @@ def emit_run_completed( "warnings": warnings or [], }, ) + + +def emit_llm_error( + *, + category: str, + severity: str, + message: str, + attempts: int = 0, + status_code: int | None = None, + retryable: bool | None = None, +) -> None: + """Emit an LLM error event for user notification. + + Args: + category: Error category ("rate_limit", "timeout", "auth", etc.) + severity: "transient" (retrying) or "fatal" (giving up) + message: Human-readable error message + attempts: Current retry attempt number (for transient errors) + status_code: HTTP status code if available + retryable: Explicit retryable flag (defaults based on severity) + """ + data: dict[str, Any] = { + "category": category, + "severity": severity, + "message": message, + } + + if attempts > 0: + data["attempts"] = attempts + + if status_code is not None: + data["status_code"] = status_code + + if retryable is not None: + data["retryable"] = retryable + elif severity == "transient": + data["retryable"] = True + elif severity == "fatal": + data["retryable"] = False + + _emit("llm.error", data) + diff --git a/src/agent/streaming/events.py b/src/agent/streaming/events.py index 5672076..2445dd8 100644 --- a/src/agent/streaming/events.py +++ b/src/agent/streaming/events.py @@ -22,6 +22,7 @@ "tool.interrupted", "input.required", "validation.result", + "llm.error", ] diff --git a/src/agent/streaming/runner.py b/src/agent/streaming/runner.py index 694ed14..cefe04e 100644 --- a/src/agent/streaming/runner.py +++ b/src/agent/streaming/runner.py @@ -8,6 +8,8 @@ from itertools import count from typing import Any +from langgraph.errors import GraphRecursionError + from agent.graph import build_graph from agent.streaming.events import EventType, StreamEvent from agent.streaming.redaction import sanitize_dict @@ -141,7 +143,19 @@ async def stream_graph( if event: yield _custom_event_line(event, seq_counter) + except GraphRecursionError as exc: + # Agent exceeded step budget (stuck in a loop) + yield _event_line( + "run.failed", + { + "message": "Agent exceeded its step budget — it may have been stuck in a loop.", + "detail": "GraphRecursionError", + "recoverable": False, + }, + seq_counter, + ) except Exception as exc: + # Generic exception handling yield _event_line( "run.failed", { diff --git a/src/agent/tools/glob_tool/glob.py b/src/agent/tools/glob_tool/glob.py index 26c30d8..87b8641 100644 --- a/src/agent/tools/glob_tool/glob.py +++ b/src/agent/tools/glob_tool/glob.py @@ -94,9 +94,13 @@ def _run( for file in matches: if ignores.is_ignored(Path(file)): continue - stat = os.stat(file) - mtime = stat.st_mtime - files.append({"path": str(Path(file).resolve()), "mtime": mtime}) + try: + stat = os.stat(file) + mtime = stat.st_mtime + files.append({"path": str(Path(file).resolve()), "mtime": mtime}) + except (FileNotFoundError, PermissionError): + # Skip broken symlinks, race-deleted files, permission-restricted files + continue files.sort(key=lambda f: f["mtime"], reverse=True) diff --git a/src/agent/tools/grep_tool/grep.py b/src/agent/tools/grep_tool/grep.py index 5a050b6..2b37e1b 100644 --- a/src/agent/tools/grep_tool/grep.py +++ b/src/agent/tools/grep_tool/grep.py @@ -91,7 +91,15 @@ def _run( if not pattern: raise ValueError("pattern is required") - regex = re.compile(pattern) + try: + regex = re.compile(pattern) + except re.error as e: + # Invalid regex from LLM - return as data so LLM can fix it + return { + "title": pattern, + "metadata": {"search_path": str(search_path), "error": str(e)}, + "output": f"Invalid regex pattern: {e}", + } matches = [] found_enough = False diff --git a/src/agent/tools/ignore_filter.py b/src/agent/tools/ignore_filter.py index 3a970ae..899aad1 100644 --- a/src/agent/tools/ignore_filter.py +++ b/src/agent/tools/ignore_filter.py @@ -46,9 +46,14 @@ def _load(root: Path) -> List[Tuple[Path, pathspec.GitIgnoreSpec]]: gi_path = Path(dirpath) / ".gitignore" try: lines = gi_path.read_text(encoding="utf-8", errors="ignore").splitlines() + spec = pathspec.GitIgnoreSpec.from_lines(lines) + specs.append((Path(dirpath), spec)) except OSError: + # Can't read the gitignore - skip it + continue + except Exception: + # Malformed gitignore (pathspec parsing error) - skip it continue - specs.append((Path(dirpath), pathspec.GitIgnoreSpec.from_lines(lines))) return specs def is_ignored(self, path: Path, is_dir: bool = False) -> bool: diff --git a/tests/unit/agent/exceptions/__init__.py b/tests/unit/agent/exceptions/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/unit/agent/graph/nodes/test_writer.py b/tests/unit/agent/graph/nodes/test_writer.py index edc0e81..63aab45 100644 --- a/tests/unit/agent/graph/nodes/test_writer.py +++ b/tests/unit/agent/graph/nodes/test_writer.py @@ -1,7 +1,8 @@ """Unit tests for the deterministic writer node.""" from agent.graph.nodes.writer import _resolve_output_path, write_yaml - +from agent.graph.nodes import writer as writer_module +import pytest class TestResolveOutputPath: def test_returns_path_when_no_conflict(self, tmp_path): @@ -62,3 +63,45 @@ def test_does_not_overwrite(self, tmp_path): assert path == str(tmp_path / ".gitlab-ci-2.yml") assert (tmp_path / ".gitlab-ci.yml").read_text() == "original" assert (tmp_path / ".gitlab-ci-2.yml").read_text() == "new content" + + +class TestWriterNodeFailure: + + def test_disk_failure_emits_run_failed_with_draft(self, monkeypatch, tmp_path): + + emitted = [] + monkeypatch.setattr( + "agent.graph.nodes.writer.get_stream_writer", + lambda: (lambda payload: emitted.append(payload)), + ) + + # Replace the write_tool reference with a stub whose invoke fails. + # (Can't monkeypatch .invoke directly — StructuredTool is a frozen + # Pydantic model.) + class _FailingTool: + def invoke(self, payload): + raise OSError("disk full") + + monkeypatch.setattr(writer_module, "write_tool", _FailingTool()) + + state = { + "project_path": str(tmp_path), + "current_yaml_draft": "name: CI\non: push\n", + "planned_filepath": ".github/workflows/ci.yml", + "pipeline_plan": [], + "validator_warnings": [], + } + + + + with pytest.raises(OSError): + writer_module.writer_node(state) + + # Exactly one event emitted, and it carries the draft for recovery. + assert len(emitted) == 1 + payload = emitted[0] + assert payload["type"] == "run.failed" + assert payload["node"] == "writer" + assert payload["data"]["detail"] == "OSError" + assert payload["data"]["draft_content"] == "name: CI\non: push\n" + assert "ci.yml" in payload["data"]["resolved_path"] diff --git a/tests/unit/agent/graph/subgraphs/test_analyzer.py b/tests/unit/agent/graph/subgraphs/test_analyzer.py index 888b165..a74ed60 100644 --- a/tests/unit/agent/graph/subgraphs/test_analyzer.py +++ b/tests/unit/agent/graph/subgraphs/test_analyzer.py @@ -158,3 +158,24 @@ def fake_run_analyzer(**kwargs): assert result["pending_question"]["type"] == "permission" assert "allow' or 'deny'" in result["pending_question"]["message"] assert result["pending_question"]["metadata"]["request_id"] == "req-current" + + +def test_run_analyzer_returns_degraded_answer_on_generic_exception(monkeypatch): + """A non-permission error (e.g. transient LLM failure) must not crash the + analyzer — it returns a degraded answer the planner/generator can use.""" + + def _raise(self, payload): + raise RuntimeError("simulated provider outage") + + fake_compiled = type("FakeAgent", (), {"invoke": _raise})() + monkeypatch.setattr( + analyzer_module, "build_analyzer_agent", lambda *a, **k: fake_compiled + ) + + result = analyzer_module.run_analyzer( + project_path="d:/repo", query="inspect", llm=object() + ) + + assert result["permission_request"] is None + assert "Analysis failed" in result["answer"] + assert "RuntimeError" in result["answer"] diff --git a/tests/unit/agent/llm/test_factory.py b/tests/unit/agent/llm/test_factory.py index 6762353..5a39889 100644 --- a/tests/unit/agent/llm/test_factory.py +++ b/tests/unit/agent/llm/test_factory.py @@ -5,6 +5,7 @@ import pytest from agent.api.schemas.streaming import LLMConfig +from agent.exceptions import AgentConfigError from agent.llm.factory import build_llm, _litellm_model, _make_hashable_config @@ -78,6 +79,7 @@ def test_builds_with_model_and_temperature(self, mock_chat_litellm): mock_chat_litellm.assert_called_once_with( model="openai/gpt-4o", temperature=0.5, + num_retries=3, api_key="sk-test", ) @@ -98,6 +100,7 @@ def test_forwards_base_url_as_api_base(self, mock_chat_litellm): mock_chat_litellm.assert_called_once_with( model="openai/llama-3.3-70b", temperature=0, + num_retries=3, api_base="http://localhost:11434/v1", ) @@ -179,3 +182,27 @@ def test_none_api_key_handled_correctly(self): ) hash_tuple = _make_hashable_config(config) assert hash_tuple[2] == "" # None converted to empty string + + +class TestFactoryErrors: + """Bad config must raise AgentConfigError with an actionable message, + not a raw KeyError / opaque ChatLiteLLM error.""" + + def test_unknown_provider_raises_agent_config_error_and_lists_providers(self): + # LLMConfig doesn't validate provider, so a bogus one reaches the factory. + cfg = LLMConfig(provider="groq", model="x", api_key="k") + cfg.provider = "not-a-real-provider" + with pytest.raises(AgentConfigError, match="Unknown provider") as exc_info: + _litellm_model(cfg) + msg = str(exc_info.value) + for known in ("openai", "anthropic", "gemini", "groq"): + assert known in msg + + def test_construction_failure_wraps_as_agent_config_error(self): + """If ChatLiteLLM raises during construction, surface AgentConfigError.""" + cfg = LLMConfig( + provider="openai", model="gpt-4o-unique-1", api_key="sk-test-unique-1" + ) + with patch("agent.llm.factory.ChatLiteLLM", side_effect=RuntimeError("boom")): + with pytest.raises(AgentConfigError, match="Failed to build LLM"): + build_llm(cfg) diff --git a/tests/unit/agent/llm/test_resilience.py b/tests/unit/agent/llm/test_resilience.py new file mode 100644 index 0000000..aae0f7a --- /dev/null +++ b/tests/unit/agent/llm/test_resilience.py @@ -0,0 +1,202 @@ +"""Unit tests for the LLM resilience layer (classification + retry wrapper). + +These cover the core of the Tier-1 exception-handling plan: + - classify_llm_error(): maps provider exceptions → (severity, category, status) + - build_resilient_llm(): structured output with .with_retry() + llm.error events +""" + +from unittest.mock import MagicMock + +import pytest +from langchain_core.exceptions import OutputParserException +from litellm.exceptions import ( + APIConnectionError, + AuthenticationError, + BadRequestError, + NotFoundError, + PermissionDeniedError, + RateLimitError, + ServiceUnavailableError, + Timeout, +) +from pydantic import BaseModel + +from agent.llm import resilience + + + +class _Schema(BaseModel): + """Minimal pydantic schema for structured-output tests.""" + + answer: str + + +# litellm exceptions want (message, llm_provider, model[, response]). Some +# require a non-Optional response whose underlying openai exception also +# reads response.request/body, so we build a lightweight stand-in. +class _FakeResponse: + def __init__(self, status_code: int): + self.status_code = status_code + self.text = "fake body" + self.headers = {} + self.request = MagicMock() + self.http_request = self.request + + +def _exc(cls, status_code=None, **extra): + """Construct a litellm exception with the minimum required kwargs.""" + kwargs = {"message": "err", "llm_provider": "openai", "model": "openai/gpt-4o"} + kwargs.update(extra) + if "response" not in kwargs and cls is PermissionDeniedError: + kwargs["response"] = _FakeResponse(status_code or 403) + exc = cls(**kwargs) + if status_code is not None: + exc.status_code = status_code + return exc + + +def _make_rate_limit(): + return _exc(RateLimitError, status_code=429) + + +def _make_auth(): + return _exc(AuthenticationError, status_code=401) + + +def _make_forbidden(): + return _exc(PermissionDeniedError, status_code=403) + + +# --------------------------------------------------------------------------- +# classify_llm_error +# --------------------------------------------------------------------------- + + +class TestClassifyLlmError: + + @pytest.mark.parametrize( + "exc_factory,expected", + [ + # parse — transient + (lambda: OutputParserException("bad schema"), ("transient", "parse", None)), + (lambda: ValueError("bad value"), ("transient", "parse", None)), + # rate_limit — transient + (_make_rate_limit, ("transient", "rate_limit", 429)), + # timeout — transient + (lambda: _exc(Timeout), ("transient", "timeout", None)), + # unavailable — transient + (lambda: _exc(ServiceUnavailableError), ("transient", "unavailable", None)), + (lambda: _exc(APIConnectionError), ("transient", "unavailable", None)), + # auth — fatal + (_make_auth, ("fatal", "auth", 401)), + (_make_forbidden, ("fatal", "auth", 403)), + # bad_request — fatal + (lambda: _exc(BadRequestError, status_code=400), ("fatal", "bad_request", 400)), + (lambda: _exc(NotFoundError, status_code=404), ("fatal", "bad_request", 404)), + ], + ) + def test_classifies_known_types(self, exc_factory, expected): + severity, category, status_code = resilience.classify_llm_error(exc_factory()) + assert (severity, category) == (expected[0], expected[1]) + # status_code may be provider-dependent; assert only when the plan pins it + if expected[2] is not None: + assert status_code == expected[2] + + def test_unknown_exception_is_fatal(self): + severity, category, status_code = resilience.classify_llm_error( + RuntimeError("something unexpected") + ) + assert severity == "fatal" + assert category == "unknown" + + def test_5xx_status_routes_to_unavailable(self): + exc = Exception("server error") + exc.status_code = 500 # type: ignore[attr-defined] + severity, category, _ = resilience.classify_llm_error(exc) + assert (severity, category) == ("transient", "unavailable") + + def test_401_status_routes_to_auth(self): + exc = Exception("plain") + exc.status_code = 401 # type: ignore[attr-defined] + severity, category, _ = resilience.classify_llm_error(exc) + assert (severity, category) == ("fatal", "auth") + + +# --------------------------------------------------------------------------- +# build_resilient_llm — retry + notification behaviour +# --------------------------------------------------------------------------- + + +def _make_chain(llm_mock): + """Return the resilient chain built from a mocked LLM. + + `llm_mock.with_structured_output(...)` must return a MagicMock whose + `.with_retry(...)` returns the configured retry runnable. + """ + return resilience.build_resilient_llm(llm_mock, _Schema, max_retries=2) + + +class TestBuildResilientLlm: + def test_success_path_returns_value(self): + """Happy path: invoke returns the parsed value with no retry.""" + inner = MagicMock() + inner.with_retry.return_value = inner + inner.invoke.return_value = _Schema(answer="ok") + llm_mock = MagicMock() + llm_mock.with_structured_output.return_value = inner + + chain = _make_chain(llm_mock) + result = chain.invoke([]) + + assert result.answer == "ok" + # structured output + retry must both be wired + llm_mock.with_structured_output.assert_called_once_with( + _Schema, method="function_calling" + ) + inner.with_retry.assert_called_once() + + def test_retry_only_covers_specified_exception_types(self): + """with_retry must be configured with the documented retryable set.""" + inner = MagicMock() + inner.with_retry.return_value = inner + llm_mock = MagicMock() + llm_mock.with_structured_output.return_value = inner + + _make_chain(llm_mock) + + kwargs = inner.with_retry.call_args.kwargs + retryable = kwargs["retry_if_exception_type"] + # The plan's retryable set: parse + transport errors + assert OutputParserException in retryable + assert ValueError in retryable + assert RateLimitError in retryable + assert Timeout in retryable + assert ServiceUnavailableError in retryable + assert APIConnectionError in retryable + # fatal types must NOT be retried + assert AuthenticationError not in retryable + assert BadRequestError not in retryable + assert kwargs["stop_after_attempt"] == 2 + assert kwargs["wait_exponential_jitter"] is True + + def test_before_sleep_callback_emits_transient_event(self, monkeypatch): + """The before_sleep hook classifies + emits a transient llm.error event.""" + emitted = [] + monkeypatch.setattr(resilience, "emit_llm_error", lambda **kw: emitted.append(kw)) + + # Synthesize a RetryCallState-like object carrying a rate-limit exception. + retry_state = MagicMock() + retry_state.outcome.exception.return_value = _make_rate_limit() + retry_state.attempt_number = 2 + + resilience._on_retry_callback(retry_state) + + assert emitted == [ + { + "category": "rate_limit", + "severity": "transient", + "attempts": 2, + "status_code": 429, + "message": "rate_limit — retrying (2)…", + } + ] diff --git a/tests/unit/agent/streaming/test_events.py b/tests/unit/agent/streaming/test_events.py index 406a100..e25f412 100644 --- a/tests/unit/agent/streaming/test_events.py +++ b/tests/unit/agent/streaming/test_events.py @@ -40,6 +40,7 @@ def test_event_type_includes_expected_public_events() -> None: "tool.interrupted", "input.required", "validation.result", + "llm.error", } assert set(get_args(EventType)) == expected diff --git a/tests/unit/agent/streaming/test_runner.py b/tests/unit/agent/streaming/test_runner.py index 431e5d5..bdc3d9f 100644 --- a/tests/unit/agent/streaming/test_runner.py +++ b/tests/unit/agent/streaming/test_runner.py @@ -1,12 +1,10 @@ """Tests for the streaming graph runner.""" from __future__ import annotations - import json from types import SimpleNamespace - +from langgraph.errors import GraphRecursionError import pytest - from agent.streaming import runner @@ -139,3 +137,48 @@ async def test_request_cancel_is_scoped_to_run_id(monkeypatch): with pytest.raises(StopAsyncIteration): await anext(stream) + + +class _RaisingGraph: + """Fake graph whose astream raises a given exception mid-stream.""" + + def __init__(self, exc): + self.exc = exc + + async def astream(self, graph_input, config, **kwargs): + if False: # pragma: no cover - keeps it an async generator + yield + raise self.exc + + +@pytest.mark.asyncio +async def test_stream_graph_recursion_error_emits_stuck_in_loop(monkeypatch): + + monkeypatch.setattr(runner, "graph", _RaisingGraph(GraphRecursionError("limit hit"))) + + lines = await _collect_lines( + runner.stream_graph({}, {"configurable": {"thread_id": "t"}}, "t", "r") + ) + + types = [line["type"] for line in lines] + assert types[0] == "run.started" + assert types[-1] == "run.failed" + failed = lines[-1] + assert failed["data"]["detail"] == "GraphRecursionError" + assert "step budget" in failed["data"]["message"] + assert failed["data"]["recoverable"] is False + + +@pytest.mark.asyncio +async def test_stream_graph_generic_exception_emits_run_failed(monkeypatch): + """Any other exception still funnels into the generic run.failed net.""" + monkeypatch.setattr(runner, "graph", _RaisingGraph(RuntimeError("boom"))) + + lines = await _collect_lines( + runner.stream_graph({}, {"configurable": {"thread_id": "t"}}, "t", "r") + ) + + failed = lines[-1] + assert failed["type"] == "run.failed" + assert failed["data"]["detail"] == "RuntimeError" + assert "boom" in failed["data"]["message"] diff --git a/tests/unit/agent/tools/test_glob_tool.py b/tests/unit/agent/tools/test_glob_tool.py index 753041a..badf297 100644 --- a/tests/unit/agent/tools/test_glob_tool.py +++ b/tests/unit/agent/tools/test_glob_tool.py @@ -1,7 +1,8 @@ """Unit tests for glob_tool.""" import pytest from agent.tools.glob_tool import create_glob_tool - +import os +import unittest.mock as mock class TestGlobTool: def test_finds_matching_py_files(self, tmp_path): @@ -136,4 +137,36 @@ def test_recursive_glob_skips_ignored_dirs(self, tmp_path): assert result["metadata"]["count"] == 1 assert "src" in result["output"] - assert "build" not in result["output"] \ No newline at end of file + assert "build" not in result["output"] + + +class TestGlobErrorHandling: + def test_stat_failure_is_skipped_not_raised(self, tmp_path): + """An os.stat failure (broken symlink, race-deleted, permission-denied) + on one file must not nullify the whole glob result. + + We simulate a dangling symlink's FileNotFoundError via a mock so the + test is cross-platform (Windows can't create symlinks without admin). + """ + + + (tmp_path / "good.py").write_text("code") + real_stat = os.stat + + def _raising_stat(path, *a, **k): + if os.fspath(path).endswith("badlink.py"): + raise FileNotFoundError("missing") + return real_stat(path, *a, **k) + + # Force a match on "badlink.py" by creating a real file that the glob + # discovers, then make its stat raise — exactly the race/symlink case. + (tmp_path / "badlink.py").write_text("code") + + glob_tool = create_glob_tool() + with mock.patch("agent.tools.glob_tool.glob.os.stat", side_effect=_raising_stat): + result = glob_tool.invoke({"pattern": "*.py", "path": str(tmp_path)}) + + # The bad file was skipped; good.py still resolves. + assert result["metadata"]["count"] == 1 + assert "good.py" in result["output"] + assert "badlink.py" not in result["output"] \ No newline at end of file diff --git a/tests/unit/agent/tools/test_grep_tool.py b/tests/unit/agent/tools/test_grep_tool.py index c42fcc4..da42f10 100644 --- a/tests/unit/agent/tools/test_grep_tool.py +++ b/tests/unit/agent/tools/test_grep_tool.py @@ -135,4 +135,22 @@ def test_explicit_path_overrides_working_directory(self, tmp_path): assert result["metadata"]["count"] == 1 assert "other_file.py" in result["output"] assert "in other" in result["output"] - assert "project_file.py" not in result["output"] \ No newline at end of file + assert "project_file.py" not in result["output"] + + +class TestGrepErrorHandling: + def test_invalid_regex_returns_data_not_raises(self, tmp_path): + """A malformed regex from the LLM must be returned as data, not raised.""" + grep_tool = create_grep_tool() + result = grep_tool.invoke({"pattern": "[unclosed", "path": str(tmp_path)}) + + # Must return a dict (never raise) so the ReAct loop can react. + assert isinstance(result, dict) + assert "Invalid regex" in result["output"] + assert result["metadata"].get("error") is not None + + def test_empty_pattern_is_rejected_before_compile(self, tmp_path): + """Empty pattern is an explicit precondition failure, not a regex issue.""" + grep_tool = create_grep_tool() + with pytest.raises(ValueError): + grep_tool.invoke({"pattern": "", "path": str(tmp_path)}) \ No newline at end of file From b919f6ef5f0728ff59580c972596dafab7d65636 Mon Sep 17 00:00:00 2001 From: Hemdan47 Date: Wed, 24 Jun 2026 21:47:05 +0300 Subject: [PATCH 7/8] refactor(llm): migrate from langchain `with_retry` to centralized tenacity policy. - Replace LangChain's `.with_retry()` with a custom `_ResilientRunnable` wrapper using Tenacity `@retry`. - Centralize shared Layer-2 tenacity configuration in `retry_policy()`, which handles retryable exceptions, exponential backoff/jitter, and emitting transient `llm.error` events. - Apply the centralized `retry_policy` to wrap the ReAct loop in the analyzer subgraph (`run_analyzer`), ensuring standard retries and notifications cover the entire agent invocation. - Introduce `tests/integration/helpers.py` to unify LLM config resolution and provider key extraction. - Refactor planner, generator, analyzer, and stream router integration tests to use the new unified helper, eliminating duplicated boilerplate. - Update unit tests in `test_resilience.py` to assert Tenacity retry execution paths, event hooks, and fatal error propagation. --- src/agent/graph/nodes/planner.py | 2 +- src/agent/graph/subgraphs/analyzer.py | 16 ++- .../generator_reflexion/generator.py | 2 +- src/agent/llm/resilience.py | 91 +++++++++++---- src/agent/streaming/runner.py | 2 +- .../agent/api/test_streaming_routes.py | 27 ++++- .../agent/graph/nodes/test_planner.py | 51 +-------- .../generator_reflexion/test_generator.py | 21 ++-- .../agent/graph/subgraphs/test_analyzer.py | 70 ++---------- .../integration/agent/graph/test_hitl_flow.py | 50 +-------- tests/integration/helpers.py | 75 +++++++++++++ tests/unit/agent/llm/test_resilience.py | 104 ++++++++++++------ 12 files changed, 283 insertions(+), 228 deletions(-) create mode 100644 tests/integration/helpers.py diff --git a/src/agent/graph/nodes/planner.py b/src/agent/graph/nodes/planner.py index d3fbe6b..376c7b8 100644 --- a/src/agent/graph/nodes/planner.py +++ b/src/agent/graph/nodes/planner.py @@ -141,7 +141,7 @@ def planner_node(state: State, config: RunnableConfig) -> dict: } llm = get_llm_from_config(config) - structured_llm = build_resilient_llm(llm, PlannerOutput) + structured_llm = build_resilient_llm(llm, PlannerOutput, max_retries=5) target_platform = state.get("target_platform", "github_actions") context_block = _build_context_block(state) diff --git a/src/agent/graph/subgraphs/analyzer.py b/src/agent/graph/subgraphs/analyzer.py index 677aa83..21ca724 100644 --- a/src/agent/graph/subgraphs/analyzer.py +++ b/src/agent/graph/subgraphs/analyzer.py @@ -12,11 +12,13 @@ from langchain.agents import create_agent from langchain_core.messages import HumanMessage from langchain_core.runnables import RunnableConfig +from tenacity import retry from agent.graph.prompts import ANALYZER_SYSTEM_PROMPT from agent.graph.schemas import AnalyzerResultEntry, PendingQuestion from agent.graph.state import State from agent.llm.helpers import get_llm_from_config +from agent.llm.resilience import retry_policy from agent.tools.permission.protocol import parse_allow_deny from agent.exceptions import PermissionRequiredException from agent.tools import ( @@ -107,12 +109,20 @@ def run_analyzer( The agent's final text answer. """ agent = build_analyzer_agent(project_path, llm=llm, approved_commands=approved_commands) - + + # Layer-2 retry around the ReAct loop. Each attempt runs the full agent + # invoke; retryable transport/parse errors back off (spanning a provider + # rate-limit window) and emit "retrying (N)..." via the shared policy's + # before_sleep callback. Final failure falls through to the except below. + @retry(**retry_policy()) + def _invoke_agent(): + return agent.invoke({"messages": [HumanMessage(content=query)]}) + answer = "" permission_request = None - + try: - result = agent.invoke({"messages": [HumanMessage(content=query)]}) + result = _invoke_agent() messages = result.get("messages", []) answer = _normalize_message_content(messages[-1].content) if messages else "" except PermissionRequiredException as e: diff --git a/src/agent/graph/subgraphs/generator_reflexion/generator.py b/src/agent/graph/subgraphs/generator_reflexion/generator.py index 501fcaf..027fc22 100644 --- a/src/agent/graph/subgraphs/generator_reflexion/generator.py +++ b/src/agent/graph/subgraphs/generator_reflexion/generator.py @@ -81,7 +81,7 @@ def generator_node(state: State, config: RunnableConfig) -> dict: Returns a partial state dict — only the fields this node writes. """ llm = get_llm_from_config(config) - structured_llm = build_resilient_llm(llm, GeneratorOutput) + structured_llm = build_resilient_llm(llm, GeneratorOutput, max_retries=5) target_platform = state.get("target_platform", "github_actions") diff --git a/src/agent/llm/resilience.py b/src/agent/llm/resilience.py index b57cc85..4d53a63 100644 --- a/src/agent/llm/resilience.py +++ b/src/agent/llm/resilience.py @@ -5,7 +5,7 @@ 2. Resilient LLM construction: Wraps structured output with retry + notification 3. Two-layer retry strategy: - Layer 1 (LiteLLM num_retries): Silent transport-level retry - - Layer 2 (LangChain .with_retry()): Visible application-level retry with notification + - Layer 2 (tenacity retry): Visible application-level retry with notification """ from __future__ import annotations @@ -26,10 +26,16 @@ Timeout, ) from pydantic import BaseModel -from tenacity import RetryCallState +from tenacity import ( + RetryCallState, + retry, + retry_if_exception_type, + stop_after_attempt, + wait_exponential_jitter, +) -# Retryable exception types for Layer 2 (.with_retry()) +# Retryable exception types for Layer 2 (tenacity) # These include both parse errors and transport errors that survived Layer 1 _RETRYABLE_EXCEPTIONS = ( # Parse errors (schema mismatch) @@ -98,25 +104,50 @@ def classify_llm_error(exc: Exception) -> tuple[str, str, int | None]: def _on_retry_callback(retry_state: RetryCallState) -> None: - """Emit a transient llm.error event on each retry attempt. - - This callback fires during Layer 2 retries (.with_retry()), providing - user notification that a sustained issue is being retried. + """Emit a transient llm.error event between retry attempts. + + Tenacity calls this via ``before_sleep`` - fires after a failed attempt + but before the backoff sleep, giving the user a "retrying (N)..." notice. """ - exc = retry_state.outcome.exception() - severity, category, status_code = classify_llm_error(exc) attempt = retry_state.attempt_number - + _, category, status_code = classify_llm_error(exc) + emit_llm_error( category=category, severity="transient", # Always transient during retry attempts=attempt, status_code=status_code, - message=f"{category} — retrying ({attempt})…", + message=f"{category} - retrying ({attempt})...", ) +def retry_policy(max_attempts: int = 6) -> dict[str, Any]: + """Shared Layer-2 tenacity config (visible retry + notification). + + Centralizing this guarantees every retry site - structured-output nodes + (planner/generator via build_resilient_llm) AND the analyzer's ReAct loop + - use the same exception list, backoff, and, crucially, the + ``before_sleep`` callback that emits the "retrying (N)..." event. Without + a single source of truth the callback is easy to forget at a call site. + + Args: + max_attempts: Total attempts (attempts, not retries). 6 attempts with + ``wait_exponential_jitter(max=30)`` sleep ~1+2+4+8+16+30 ~= 61s, + which spans a typical 40-RPM provider window. + + Returns: + kwargs dict to splat into ``tenacity.retry(**retry_policy())``. + """ + return { + "retry": retry_if_exception_type(_RETRYABLE_EXCEPTIONS), + "stop": stop_after_attempt(max_attempts), + "wait": wait_exponential_jitter(max=30), + "before_sleep": _on_retry_callback, + "reraise": True, + } + + def build_resilient_llm( llm: BaseChatModel, schema: type[BaseModel], @@ -133,7 +164,7 @@ def build_resilient_llm( - Respects provider Retry-After headers - User sees nothing if it succeeds - Layer 2 (LangChain .with_retry(), this function): + Layer 2 (tenacity retry, this function): - Catches transport errors that exhausted Layer 1 (sustained issues) - Also catches parse errors (schema mismatch) - Emits llm.error events via before_sleep callback @@ -149,15 +180,31 @@ def build_resilient_llm( Combined with num_retries=3, max 6 total HTTP attempts Returns: - A Runnable that parses into `schema` with built-in resilience + A Runnable that parses into ``schema`` with built-in resilience """ - return ( - llm.with_structured_output(schema, method="function_calling") - .with_retry( - retry_if_exception_type=_RETRYABLE_EXCEPTIONS, - stop_after_attempt=max_retries, - wait_exponential_jitter=True, - before_sleep=_on_retry_callback, - ) - ) + base_runnable = llm.with_structured_output(schema, method="function_calling") + + # Layer-2 tenacity settings shared by both sync and async paths. + # max_retries is retries, retry_policy takes total attempts -> +1. + _retry_kwargs = retry_policy(max_retries + 1) + + @retry(**_retry_kwargs) + def _invoke(input: Any, config: Any = None, **kwargs: Any) -> Any: + return base_runnable.invoke(input, config, **kwargs) + + @retry(**_retry_kwargs) + async def _ainvoke(input: Any, config: Any = None, **kwargs: Any) -> Any: + return await base_runnable.ainvoke(input, config, **kwargs) + + class _ResilientRunnable: + """Thin Runnable-compatible wrapper backed by tenacity retry.""" + + def invoke(self, input: Any, config: Any = None, **kwargs: Any) -> Any: + return _invoke(input, config, **kwargs) + + async def ainvoke(self, input: Any, config: Any = None, **kwargs: Any) -> Any: + return await _ainvoke(input, config, **kwargs) + + return _ResilientRunnable() + diff --git a/src/agent/streaming/runner.py b/src/agent/streaming/runner.py index cefe04e..cfb43b9 100644 --- a/src/agent/streaming/runner.py +++ b/src/agent/streaming/runner.py @@ -148,7 +148,7 @@ async def stream_graph( yield _event_line( "run.failed", { - "message": "Agent exceeded its step budget — it may have been stuck in a loop.", + "message": "Agent exceeded its step budget - it may have been stuck in a loop.", "detail": "GraphRecursionError", "recoverable": False, }, diff --git a/tests/integration/agent/api/test_streaming_routes.py b/tests/integration/agent/api/test_streaming_routes.py index 5010dcb..f06d812 100644 --- a/tests/integration/agent/api/test_streaming_routes.py +++ b/tests/integration/agent/api/test_streaming_routes.py @@ -13,12 +13,25 @@ from agent.streaming import runner as streaming_runner +class _FakeState: + """Stand-in for the object returned by graph.get_state(). + + The resume route checks ``state.values`` to decide whether a thread exists; + a non-empty dict is treated as an existing thread. + """ + + def __init__(self, values: dict[str, Any] | None = None) -> None: + self.values = values if values is not None else {"messages": []} + + class FakeGraph: def __init__(self, chunks: list[dict[str, Any]]) -> None: self.chunks = chunks self.graph_input: Any = None self.config: dict[str, Any] | None = None self.kwargs: dict[str, Any] | None = None + # Non-empty values so the resume route treats the thread as existing. + self._state = _FakeState() async def astream(self, graph_input, config, **kwargs): self.graph_input = graph_input @@ -27,6 +40,9 @@ async def astream(self, graph_input, config, **kwargs): for chunk in self.chunks: yield chunk + def get_state(self, config): + return self._state + def _parse_ndjson(response: httpx.Response) -> list[dict[str, Any]]: return [ @@ -139,7 +155,10 @@ async def test_resume_stream_endpoint_uses_command_and_skips_run_started( } ] ) + # Patch BOTH bindings: the runner module global (used by stream_graph) + # and the route module's own import (used by resume_stream's get_state). monkeypatch.setattr(streaming_runner, "graph", fake_graph) + monkeypatch.setattr(streaming_route, "graph", fake_graph) transport = httpx.ASGITransport(app=app) async with httpx.AsyncClient(transport=transport, base_url="http://test") as client: @@ -192,5 +211,11 @@ async def test_cancel_endpoint_reports_scoped_status(monkeypatch: pytest.MonkeyP active = await client.post("/generate/runs/run-active/cancel") missing = await client.post("/generate/runs/run-missing/cancel") + assert active.status_code == 200 assert active.json() == {"status": "cancelling", "run_id": "run-active"} - assert missing.json() == {"status": "not_found", "run_id": "run-missing"} + + # Unknown run now raises HTTPException(404) rather than returning a body. + assert missing.status_code == 404 + assert missing.json() == { + "detail": "Run 'run-missing' not found or already completed." + } diff --git a/tests/integration/agent/graph/nodes/test_planner.py b/tests/integration/agent/graph/nodes/test_planner.py index 2ef83ff..2ef9204 100644 --- a/tests/integration/agent/graph/nodes/test_planner.py +++ b/tests/integration/agent/graph/nodes/test_planner.py @@ -9,7 +9,7 @@ Requires: A valid LLM API key configured in .env """ -import os + import pytest from langchain_core.messages import HumanMessage @@ -17,56 +17,13 @@ from agent.graph.schemas import AnalyzerResultEntry -# Skip all tests if no API key set -pytestmark = pytest.mark.skipif( - not os.environ.get("GROQ_API_KEY") - and not os.environ.get("OPENAI_API_KEY") - and not os.environ.get("GOOGLE_API_KEY") - and not os.environ.get("ANTHROPIC_API_KEY"), - reason="No LLM API key configured — skipping integration tests", -) - - -# --------------------------------------------------------------------------- -# Helpers -# --------------------------------------------------------------------------- - -def _get_llm_config() -> dict: - """Return LLM config based on available API keys.""" - if os.environ.get("GROQ_API_KEY"): - return { - "provider": "groq", - "model": "llama-3.3-70b-versatile", - "api_key": os.environ["GROQ_API_KEY"], - "temperature": 0, - } - elif os.environ.get("OPENAI_API_KEY"): - return { - "provider": "openai", - "model": "gpt-4o", - "api_key": os.environ["OPENAI_API_KEY"], - "temperature": 0, - } - elif os.environ.get("GOOGLE_API_KEY"): - return { - "provider": "gemini", - "model": "gemini-1.5-pro", - "api_key": os.environ["GOOGLE_API_KEY"], - "temperature": 0, - } - elif os.environ.get("ANTHROPIC_API_KEY"): - return { - "provider": "anthropic", - "model": "claude-3-5-sonnet-20241022", - "api_key": os.environ["ANTHROPIC_API_KEY"], - "temperature": 0, - } - raise ValueError("No API key available") +from tests.integration.helpers import get_llm_config def _config() -> dict: """Return a minimal RunnableConfig with llm_config.""" - return {"configurable": {"llm_config": _get_llm_config()}} + return {"configurable": {"llm_config": get_llm_config()}} + def _base_state(**overrides) -> dict: """Return a minimal valid State dict for the planner.""" diff --git a/tests/integration/agent/graph/subgraphs/generator_reflexion/test_generator.py b/tests/integration/agent/graph/subgraphs/generator_reflexion/test_generator.py index 0a63f7a..8446a72 100644 --- a/tests/integration/agent/graph/subgraphs/generator_reflexion/test_generator.py +++ b/tests/integration/agent/graph/subgraphs/generator_reflexion/test_generator.py @@ -10,24 +10,19 @@ Requires: A valid LLM API key configured in .env """ -import os - import pytest import yaml from langchain_core.messages import HumanMessage from agent.graph.subgraphs.generator_reflexion.generator import generator_node from agent.graph.schemas import AnalyzerResultEntry +from tests.integration.helpers import get_llm_config + +def _config() -> dict: + """Return a minimal RunnableConfig with llm_config.""" + return {"configurable": {"llm_config": get_llm_config()}} -# Skip all tests if no API key set -pytestmark = pytest.mark.skipif( - not os.environ.get("GROQ_API_KEY") - and not os.environ.get("OPENAI_API_KEY") - and not os.environ.get("GOOGLE_API_KEY") - and not os.environ.get("ANTHROPIC_API_KEY"), - reason="No LLM API key configured — skipping integration tests", -) def _base_state(**overrides) -> dict: @@ -91,7 +86,7 @@ def test_github_actions_python_draft(self): "Has Dockerfile in project root." )], ) - result = generator_node(state) + result = generator_node(state, _config()) # If the LLM produced a draft (it might request analysis instead) if result.get("current_yaml_draft"): @@ -123,7 +118,7 @@ def test_gitlab_ci_node_draft(self): "Test runner: Jest. Package manager: npm." )], ) - result = generator_node(state) + result = generator_node(state, _config()) if result.get("current_yaml_draft"): draft = result["current_yaml_draft"] @@ -163,7 +158,7 @@ def test_fixes_validation_errors(self): ], retry_count=1, ) - result = generator_node(state) + result = generator_node(state, _config()) if result.get("current_yaml_draft"): draft = result["current_yaml_draft"] diff --git a/tests/integration/agent/graph/subgraphs/test_analyzer.py b/tests/integration/agent/graph/subgraphs/test_analyzer.py index 13f2e48..6e48c03 100644 --- a/tests/integration/agent/graph/subgraphs/test_analyzer.py +++ b/tests/integration/agent/graph/subgraphs/test_analyzer.py @@ -8,58 +8,12 @@ Requires: A valid LLM API key configured in .env """ -import os import pytest import time from agent.graph.subgraphs.analyzer import run_analyzer -from agent.llm.factory import build_llm -from agent.api.schemas.streaming import LLMConfig - - -# Skip all tests if no API key is set -pytestmark = pytest.mark.skipif( - not os.environ.get("GROQ_API_KEY") - and not os.environ.get("OPENAI_API_KEY") - and not os.environ.get("GOOGLE_API_KEY") - and not os.environ.get("ANTHROPIC_API_KEY"), - reason="No LLM API key configured — skipping integration tests", -) - - -def _get_llm(): - """Return an LLM instance based on available API keys.""" - if os.environ.get("GROQ_API_KEY"): - config = LLMConfig( - provider="groq", - model="llama-3.3-70b-versatile", - api_key=os.environ["GROQ_API_KEY"], - temperature=0, - ) - elif os.environ.get("OPENAI_API_KEY"): - config = LLMConfig( - provider="openai", - model="gpt-4o", - api_key=os.environ["OPENAI_API_KEY"], - temperature=0, - ) - elif os.environ.get("GOOGLE_API_KEY"): - config = LLMConfig( - provider="gemini", - model="gemini-1.5-pro", - api_key=os.environ["GOOGLE_API_KEY"], - temperature=0, - ) - elif os.environ.get("ANTHROPIC_API_KEY"): - config = LLMConfig( - provider="anthropic", - model="claude-3-5-sonnet-20241022", - api_key=os.environ["ANTHROPIC_API_KEY"], - temperature=0, - ) - else: - raise ValueError("No API key available") - return build_llm(config) +from tests.integration.helpers import get_test_llm + @pytest.fixture(autouse=True) @@ -169,7 +123,7 @@ def test_detects_python(self, python_flask_project): result = run_analyzer( str(python_flask_project), "What programming language is this project written in?", - llm=_get_llm(), + llm=get_test_llm(), ) assert "python" in result["answer"].lower() @@ -177,7 +131,7 @@ def test_detects_node(self, node_express_project): result = run_analyzer( str(node_express_project), "What programming language is this project written in?", - llm=_get_llm(), + llm=get_test_llm(), ) assert "javascript" in result["answer"].lower() or "node" in result["answer"].lower() @@ -185,7 +139,7 @@ def test_detects_go(self, go_project): result = run_analyzer( str(go_project), "What programming language is this project written in?", - llm=_get_llm(), + llm=get_test_llm(), ) assert "go" in result["answer"].lower() @@ -201,7 +155,7 @@ def test_finds_flask(self, python_flask_project): result = run_analyzer( str(python_flask_project), "What web framework does this project use?", - llm=_get_llm(), + llm=get_test_llm(), ) assert "flask" in result["answer"].lower() @@ -209,7 +163,7 @@ def test_finds_express(self, node_express_project): result = run_analyzer( str(node_express_project), "What web framework does this project use?", - llm=_get_llm(), + llm=get_test_llm(), ) assert "express" in result["answer"].lower() @@ -217,7 +171,7 @@ def test_finds_test_framework(self, python_flask_project): result = run_analyzer( str(python_flask_project), "What test framework is configured in this project?", - llm=_get_llm(), + llm=get_test_llm(), ) assert "pytest" in result["answer"].lower() @@ -225,7 +179,7 @@ def test_finds_node_test_runner(self, node_express_project): result = run_analyzer( str(node_express_project), "What test runner does this project use?", - llm=_get_llm(), + llm=get_test_llm(), ) assert "jest" in result["answer"].lower() @@ -241,7 +195,7 @@ def test_finds_dockerfile(self, python_flask_project): result = run_analyzer( str(python_flask_project), "Does this project have a Dockerfile? If yes, what base image does it use?", - llm=_get_llm(), + llm=get_test_llm(), ) assert "python" in result["answer"].lower() @@ -249,7 +203,7 @@ def test_finds_npm_scripts(self, node_express_project): result = run_analyzer( str(node_express_project), "What npm scripts are defined in this project?", - llm=_get_llm(), + llm=get_test_llm(), ) assert "start" in result["answer"].lower() assert "test" in result["answer"].lower() @@ -258,7 +212,7 @@ def test_finds_makefile_targets(self, go_project): result = run_analyzer( str(go_project), "What build targets are defined in the Makefile?", - llm=_get_llm(), + llm=get_test_llm(), ) assert "build" in result["answer"].lower() assert "test" in result["answer"].lower() diff --git a/tests/integration/agent/graph/test_hitl_flow.py b/tests/integration/agent/graph/test_hitl_flow.py index f6ee813..d64af81 100644 --- a/tests/integration/agent/graph/test_hitl_flow.py +++ b/tests/integration/agent/graph/test_hitl_flow.py @@ -9,7 +9,6 @@ from __future__ import annotations import json -import os import time from types import SimpleNamespace from uuid import uuid4 @@ -25,46 +24,9 @@ from agent.streaming import runner as streaming_runner -HAS_API_KEY = ( - bool(os.environ.get("GROQ_API_KEY")) - or bool(os.environ.get("OPENAI_API_KEY")) - or bool(os.environ.get("GOOGLE_API_KEY")) - or bool(os.environ.get("ANTHROPIC_API_KEY")) -) +from tests.integration.helpers import get_llm_config -def _get_llm_config() -> dict: - """Return LLM config based on available API keys.""" - if os.environ.get("GROQ_API_KEY"): - return { - "provider": "groq", - "model": "llama-3.3-70b-versatile", - "api_key": os.environ["GROQ_API_KEY"], - "temperature": 0, - } - elif os.environ.get("OPENAI_API_KEY"): - return { - "provider": "openai", - "model": "gpt-4o", - "api_key": os.environ["OPENAI_API_KEY"], - "temperature": 0, - } - elif os.environ.get("GOOGLE_API_KEY"): - return { - "provider": "gemini", - "model": "gemini-1.5-pro", - "api_key": os.environ["GOOGLE_API_KEY"], - "temperature": 0, - } - elif os.environ.get("ANTHROPIC_API_KEY"): - return { - "provider": "anthropic", - "model": "claude-3-5-sonnet-20241022", - "api_key": os.environ["ANTHROPIC_API_KEY"], - "temperature": 0, - } - raise ValueError("No API key available") - @pytest.fixture(autouse=True) def _rate_limit_delay(): @@ -129,10 +91,6 @@ def _is_transient_provider_limit(response) -> bool: @pytest.mark.asyncio -@pytest.mark.skipif( - not HAS_API_KEY, - reason="No LLM API key configured — skipping permission integration test", -) async def test_hitl_permission_pause_and_resume_with_real_llm(sample_project): prompt = ( "Generate a GitHub Actions workflow for this project. " @@ -152,7 +110,7 @@ async def test_hitl_permission_pause_and_resume_with_real_llm(sample_project): "prompt": prompt, "target_platform": "github_actions", "thread_id": f"test-{uuid4().hex[:8]}", - "llm": _get_llm_config(), + "llm": get_llm_config(), } ) assert stream_response.status_code == 200 @@ -199,7 +157,7 @@ async def test_hitl_permission_pause_and_resume_with_real_llm(sample_project): f"/generate/{thread_id}/stream/resume", json={ "response": user_reply, - "llm": _get_llm_config(), + "llm": get_llm_config(), } ) assert stream_response.status_code == 200 @@ -235,7 +193,7 @@ def test_hitl_normal_clarification_pause_and_resume_real_graph(sample_project): config = { "configurable": { "thread_id": thread_id, - "llm_config": _get_llm_config(), + "llm_config": get_llm_config(), } } diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py new file mode 100644 index 0000000..a675e35 --- /dev/null +++ b/tests/integration/helpers.py @@ -0,0 +1,75 @@ +"""Shared LLM configuration helpers for integration tests. + +Reads from the same .env variables the app uses: + LLM_PROVIDER e.g. "google_genai", "openai", "groq", "anthropic" + GENERATION_MODEL e.g. "gemini-3.1-flash-live-preview", "gpt-4o" + + the matching API key env var (GOOGLE_API_KEY, OPENAI_API_KEY, etc.) +""" + +from __future__ import annotations + +import os + +import pytest + +from agent.api.schemas.streaming import LLMConfig +from agent.llm.factory import build_llm + + +_PROVIDER_MAP: dict[str, tuple[str, str]] = { + "google_genai": ("gemini", "GOOGLE_API_KEY"), + "openai": ("openai", "OPENAI_API_KEY"), + "groq": ("groq", "GROQ_API_KEY"), + "anthropic": ("anthropic", "ANTHROPIC_API_KEY"), +} + +# Default model per provider (used when GENERATION_MODEL is not set) +_DEFAULT_MODEL: dict[str, str] = { + "gemini": "gemini-2.0-flash", + "openai": "gpt-4o", + "groq": "llama-3.3-70b-versatile", + "anthropic": "claude-3-5-sonnet-20241022", +} + + +def get_llm_config() -> dict: + """Build an LLMConfig dict from .env variables. + + Reads LLM_PROVIDER and GENERATION_MODEL, resolves the API key, + and returns a dict suitable for ``config["configurable"]["llm_config"]``. + + Raises: + pytest.skip: If the required env vars are missing. + """ + env_provider = os.environ.get("LLM_PROVIDER", "") + if env_provider not in _PROVIDER_MAP: + pytest.skip( + f"LLM_PROVIDER={env_provider!r} not recognised. " + f"Set one of: {', '.join(_PROVIDER_MAP)}" + ) + + config_provider, key_env = _PROVIDER_MAP[env_provider] + api_key = os.environ.get(key_env, "") + if not api_key: + pytest.skip(f"{key_env} not set — skipping integration tests") + + model = os.environ.get("GENERATION_MODEL") or _DEFAULT_MODEL.get(config_provider, "") + if not model: + pytest.skip("GENERATION_MODEL not set and no default available") + + return { + "provider": config_provider, + "model": model, + "api_key": api_key, + "temperature": 0, + } + + +def get_test_llm(): + """Return a built LLM instance from .env config. + + Convenience wrapper for tests that need a BaseChatModel directly + (e.g. the analyzer tests). + """ + cfg = get_llm_config() + return build_llm(LLMConfig(**cfg)) diff --git a/tests/unit/agent/llm/test_resilience.py b/tests/unit/agent/llm/test_resilience.py index aae0f7a..4add8d3 100644 --- a/tests/unit/agent/llm/test_resilience.py +++ b/tests/unit/agent/llm/test_resilience.py @@ -6,7 +6,7 @@ """ from unittest.mock import MagicMock - +import unittest.mock as _mock import pytest from langchain_core.exceptions import OutputParserException from litellm.exceptions import ( @@ -127,20 +127,15 @@ def test_401_status_routes_to_auth(self): # --------------------------------------------------------------------------- -def _make_chain(llm_mock): - """Return the resilient chain built from a mocked LLM. - - `llm_mock.with_structured_output(...)` must return a MagicMock whose - `.with_retry(...)` returns the configured retry runnable. - """ - return resilience.build_resilient_llm(llm_mock, _Schema, max_retries=2) +def _make_chain(llm_mock, max_retries=2): + """Return the resilient chain built from a mocked LLM.""" + return resilience.build_resilient_llm(llm_mock, _Schema, max_retries=max_retries) class TestBuildResilientLlm: def test_success_path_returns_value(self): """Happy path: invoke returns the parsed value with no retry.""" inner = MagicMock() - inner.with_retry.return_value = inner inner.invoke.return_value = _Schema(answer="ok") llm_mock = MagicMock() llm_mock.with_structured_output.return_value = inner @@ -149,42 +144,80 @@ def test_success_path_returns_value(self): result = chain.invoke([]) assert result.answer == "ok" - # structured output + retry must both be wired llm_mock.with_structured_output.assert_called_once_with( _Schema, method="function_calling" ) - inner.with_retry.assert_called_once() - def test_retry_only_covers_specified_exception_types(self): - """with_retry must be configured with the documented retryable set.""" + def test_retries_on_retryable_exception_then_succeeds(self, monkeypatch): + """Retryable exception on first attempt → success on second attempt.""" + emitted = [] + monkeypatch.setattr(resilience, "emit_llm_error", lambda **kw: emitted.append(kw)) + # Patch out the sleep inside the retry loop so the test is fast + + monkeypatch.setattr("time.sleep", _mock.MagicMock()) + inner = MagicMock() - inner.with_retry.return_value = inner + call_count = 0 + + def _invoke(msgs, config=None, **kw): + nonlocal call_count + call_count += 1 + if call_count == 1: + raise OutputParserException("bad schema") + return _Schema(answer="recovered") + + inner.invoke.side_effect = _invoke llm_mock = MagicMock() llm_mock.with_structured_output.return_value = inner - _make_chain(llm_mock) - - kwargs = inner.with_retry.call_args.kwargs - retryable = kwargs["retry_if_exception_type"] - # The plan's retryable set: parse + transport errors - assert OutputParserException in retryable - assert ValueError in retryable - assert RateLimitError in retryable - assert Timeout in retryable - assert ServiceUnavailableError in retryable - assert APIConnectionError in retryable - # fatal types must NOT be retried - assert AuthenticationError not in retryable - assert BadRequestError not in retryable - assert kwargs["stop_after_attempt"] == 2 - assert kwargs["wait_exponential_jitter"] is True - - def test_before_sleep_callback_emits_transient_event(self, monkeypatch): - """The before_sleep hook classifies + emits a transient llm.error event.""" + chain = resilience.build_resilient_llm(llm_mock, _Schema, max_retries=2) + result = chain.invoke([]) + + assert result.answer == "recovered" + assert call_count == 2 + assert len(emitted) == 1 + assert emitted[0]["category"] == "parse" + assert emitted[0]["severity"] == "transient" + + def test_raises_after_max_retries_exhausted(self, monkeypatch): + """After max_retries+1 attempts the exception propagates.""" + monkeypatch.setattr(resilience, "emit_llm_error", lambda **kw: None) + import unittest.mock as _mock + monkeypatch.setattr("time.sleep", _mock.MagicMock()) + + inner = MagicMock() + inner.invoke.side_effect = OutputParserException("always fails") + llm_mock = MagicMock() + llm_mock.with_structured_output.return_value = inner + + chain = resilience.build_resilient_llm(llm_mock, _Schema, max_retries=2) + with pytest.raises(OutputParserException): + chain.invoke([]) + + # 1 original attempt + 2 retries = 3 total + assert inner.invoke.call_count == 3 + + def test_non_retryable_exception_propagates_immediately(self, monkeypatch): + """Fatal (non-retryable) exceptions are NOT retried.""" + monkeypatch.setattr(resilience, "emit_llm_error", lambda **kw: None) + + inner = MagicMock() + inner.invoke.side_effect = _make_auth() + llm_mock = MagicMock() + llm_mock.with_structured_output.return_value = inner + + chain = resilience.build_resilient_llm(llm_mock, _Schema, max_retries=2) + with pytest.raises(AuthenticationError): + chain.invoke([]) + + # Must NOT retry fatal errors + assert inner.invoke.call_count == 1 + + def test_on_retry_callback_emits_transient_event(self, monkeypatch): + """_on_retry_callback classifies + emits a transient llm.error event.""" emitted = [] monkeypatch.setattr(resilience, "emit_llm_error", lambda **kw: emitted.append(kw)) - # Synthesize a RetryCallState-like object carrying a rate-limit exception. retry_state = MagicMock() retry_state.outcome.exception.return_value = _make_rate_limit() retry_state.attempt_number = 2 @@ -197,6 +230,7 @@ def test_before_sleep_callback_emits_transient_event(self, monkeypatch): "severity": "transient", "attempts": 2, "status_code": 429, - "message": "rate_limit — retrying (2)…", + "message": "rate_limit - retrying (2)...", } ] + From 9416c57439fee2a4aa869aad37362204d23ef63b Mon Sep 17 00:00:00 2001 From: Hemdan47 Date: Wed, 24 Jun 2026 21:47:40 +0300 Subject: [PATCH 8/8] Add warning filter in `pyproject.toml` to ignore cosmetic Pydantic serialization warnings. --- pyproject.toml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pyproject.toml b/pyproject.toml index 06354f4..525b0e1 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -55,4 +55,10 @@ addopts = "--basetemp=.pytest-tmp" cache_dir = ".pytest-cache" filterwarnings = [ "ignore::langgraph.warnings.LangGraphDeprecatedSinceV10", + # LangChain <-> pydantic type-widening noise: library declares base types + # (Generation/BaseMessage) but emits subclasses (ChatGeneration/AIMessage) + # at serialize time. Cosmetic, library-side, not actionable in this repo. + # Matched by message text because pydantic registers the warning class + # dynamically (not importable as pydantic.PydanticSerializationUnexpectedValue). + 'ignore:Pydantic serializer warnings:UserWarning', ]