diff --git a/pyproject.toml b/pyproject.toml index 5bb57ff..525b0e1 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -12,14 +12,13 @@ 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", "html2text>=2020.1.16", "detect-secrets>=1.5,<2", + "pathspec>=0.12.1", # Data & Config "pydantic>=2.0.0", @@ -56,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', ] 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..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"]) @@ -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), @@ -69,10 +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.""" + """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]}" - config = {"configurable": {"thread_id": thread_id}} - + return StreamingResponse( stream_graph( Command(resume=request.response), @@ -93,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/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/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 42adc3b..376c7b8 100644 --- a/src/agent/graph/nodes/planner.py +++ b/src/agent/graph/nodes/planner.py @@ -10,13 +10,15 @@ 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.streaming.emitter import emit_plan, emit_title, emit_update +from agent.llm.helpers import get_llm_from_config +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: @@ -97,7 +99,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,8 +140,8 @@ def planner_node(state: State) -> dict: "replan_count": 0, } - llm = get_generation_llm() - structured_llm = llm.with_structured_output(PlannerOutput, method="function_calling") + llm = get_llm_from_config(config) + structured_llm = build_resilient_llm(llm, PlannerOutput, max_retries=5) target_platform = state.get("target_platform", "github_actions") context_block = _build_context_block(state) @@ -153,24 +155,42 @@ def planner_node(state: State) -> 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) # 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 +199,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: 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 1597e0d..21ca724 100644 --- a/src/agent/graph/subgraphs/analyzer.py +++ b/src/agent/graph/subgraphs/analyzer.py @@ -11,18 +11,21 @@ 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.provider import get_generation_llm +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 ( create_bash_tool, - glob_tool, - grep_tool, - list_tool, + create_glob_tool, + create_grep_tool, + create_list_tool, read_tool, ) @@ -61,7 +64,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,16 +74,16 @@ 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( 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, ] @@ -106,17 +109,31 @@ 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: 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, @@ -124,7 +141,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 +155,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 +200,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..027fc22 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,8 +18,9 @@ ) from agent.graph.schemas import GeneratorOutput, PendingQuestion from agent.graph.state import State -from agent.llm.provider import get_generation_llm -from agent.streaming.emitter import emit_update +from agent.llm.helpers import get_llm_from_config +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 @@ -73,13 +75,13 @@ 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() - structured_llm = llm.with_structured_output(GeneratorOutput, method="function_calling") + llm = get_llm_from_config(config) + structured_llm = build_resilient_llm(llm, GeneratorOutput, max_retries=5) target_platform = state.get("target_platform", "github_actions") @@ -94,10 +96,22 @@ def generator_node(state: State) -> 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 new file mode 100644 index 0000000..c45d9b9 --- /dev/null +++ b/src/agent/llm/factory.py @@ -0,0 +1,101 @@ +""" +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 langchain_core.language_models import BaseChatModel +from langchain_litellm import ChatLiteLLM + +from agent.api.schemas.streaming import LLMConfig +from agent.exceptions import AgentConfigError + + +_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. + + 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}" + + +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. + + 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, + model=model, + api_key=api_key or None, + base_url=base_url or None, + temperature=temperature, + ) + + 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: + """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/src/agent/llm/resilience.py b/src/agent/llm/resilience.py new file mode 100644 index 0000000..4d53a63 --- /dev/null +++ b/src/agent/llm/resilience.py @@ -0,0 +1,210 @@ +"""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 (tenacity 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, + retry, + retry_if_exception_type, + stop_after_attempt, + wait_exponential_jitter, +) + + +# 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) + 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 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() + 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})...", + ) + + +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], + *, + 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 (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 + - 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 + + """ + 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/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..cfb43b9 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/__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/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/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..87b8641 100644 --- a/src/agent/tools/glob_tool/glob.py +++ b/src/agent/tools/glob_tool/glob.py @@ -1,95 +1,166 @@ -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 +from agent.tools.ignore_filter import IgnoreFilter -@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 = [] + 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: + # 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) + + # Drop anything ignored by .gitignore, then collect mtime. + files = [] + for file in matches: + if ignores.is_ignored(Path(file)): + continue + 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) + + 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..2b37e1b 100644 --- a/src/agent/tools/grep_tool/grep.py +++ b/src/agent/tools/grep_tool/grep.py @@ -2,17 +2,17 @@ import re import fnmatch from pathlib import Path -from langchain_core.tools import tool +from typing import Optional, Type -from agent.streaming.tool_run import tool_run +from langchain_core.tools import BaseTool +from langchain_core.callbacks.manager import CallbackManagerForToolRun +from pydantic import BaseModel, Field -# Directories to always skip -_SKIP_DIRS = { - ".git", "__pycache__", "node_modules", ".venv", "venv", - ".tox", ".mypy_cache", ".pytest_cache", "dist", "build", -} +from agent.streaming.tool_run import tool_run +from agent.tools.ignore_filter import IgnoreFilter -# 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", @@ -28,109 +28,188 @@ 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") + + 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 + + ignores = IgnoreFilter(search_path) + for root, dirs, files in os.walk(str(search_path)): + # 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 + + # 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] - ctx["result_label"] = f'Found {len(matches)} matches for "{pattern}"' - ctx["result_data"] = {"count": len(matches), "truncated": truncated} + # 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.)") - 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/ignore_filter.py b/src/agent/tools/ignore_filter.py new file mode 100644 index 0000000..899aad1 --- /dev/null +++ b/src/agent/tools/ignore_filter.py @@ -0,0 +1,94 @@ +"""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() + 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 + 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/__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..70f63e7 100644 --- a/src/agent/tools/list_tool/list.py +++ b/src/agent/tools/list_tool/list.py @@ -1,141 +1,191 @@ 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 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/", -] - -@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 = [] +from langchain_core.tools import BaseTool +from langchain_core.callbacks.manager import CallbackManagerForToolRun +from pydantic import BaseModel, Field - 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) +from agent.streaming.tool_run import tool_run +from agent.tools.ignore_filter import IgnoreFilter + + +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: + ignores = IgnoreFilter(resolved) + extra_ignore = ignore or [] + files = [] + + for root, dirs, filenames in os.walk(resolved): + # 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: + 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) + + 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) - - if parent_str == "." or parent_str == str(current): - break + current = Path(file).parent + while str(current) != ".": + parent = current.parent + parent_str = str(parent) - if parent_str and parent_str not in dirs_set: - dirs_set.add(parent_str) + if parent_str == "." or parent_str == str(current): + break - current = parent + if parent_str and parent_str not in dirs_set: + dirs_set.add(parent_str) - def render_dir(dirpath: str, depth: int = 0) -> str: - indent = " " * depth - output = "" + current = parent - if depth > 0 and dirpath: - output += f"{indent}{Path(dirpath).name}/\n" + def render_dir(dirpath: str, depth: int = 0) -> str: + indent = " " * depth + output = "" - # Subdirectories - children = sorted([ - d for d in dirs_set - if d and str(Path(d).parent) == (dirpath if dirpath else ".") - ]) + if depth > 0 and dirpath: + output += f"{indent}{Path(dirpath).name}/\n" - for child in children: - output += render_dir(child, depth + 1) + # Subdirectories + children = sorted([ + d for d in dirs_set + if d and str(Path(d).parent) == (dirpath if dirpath else ".") + ]) - # Files - for f in sorted(files_by_dir.get(dirpath, [])): - output += f"{' ' * (depth + 1)}{f}\n" + for child in children: + output += render_dir(child, depth + 1) - return output + # Files + for f in sorted(files_by_dir.get(dirpath, [])): + output += f"{' ' * (depth + 1)}{f}\n" - output = f"{resolved}/\n" + render_dir("") - truncated = len(files) >= limit + return output - ctx["result_label"] = f"Listed {len(files)} files in {resolved.name}" - ctx["result_data"] = { - "count": len(files), - "truncated": truncated, - } + output = f"{resolved}/\n" + render_dir("") + truncated = len(files) >= limit - 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/integration/agent/api/test_streaming_routes.py b/tests/integration/agent/api/test_streaming_routes.py index a92b14d..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 [ @@ -74,6 +90,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 +106,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"], @@ -123,13 +155,24 @@ 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: 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 +180,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] @@ -157,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 b53b3bb..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,19 +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", -) +from tests.integration.helpers import get_llm_config -# --------------------------------------------------------------------------- -# Helpers -# --------------------------------------------------------------------------- +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.""" @@ -77,7 +71,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 +91,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 +123,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 +149,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 +163,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 +193,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 +214,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/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 9c6d02f..6e48c03 100644 --- a/tests/integration/agent/graph/subgraphs/test_analyzer.py +++ b/tests/integration/agent/graph/subgraphs/test_analyzer.py @@ -8,22 +8,13 @@ Requires: A valid LLM API key configured in .env """ -import os import pytest import time from agent.graph.subgraphs.analyzer import run_analyzer +from tests.integration.helpers import get_test_llm -# 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", -) - @pytest.fixture(autouse=True) def _rate_limit_delay(): @@ -132,6 +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_test_llm(), ) assert "python" in result["answer"].lower() @@ -139,6 +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_test_llm(), ) assert "javascript" in result["answer"].lower() or "node" in result["answer"].lower() @@ -146,6 +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_test_llm(), ) assert "go" in result["answer"].lower() @@ -161,6 +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_test_llm(), ) assert "flask" in result["answer"].lower() @@ -168,6 +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_test_llm(), ) assert "express" in result["answer"].lower() @@ -175,6 +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_test_llm(), ) assert "pytest" in result["answer"].lower() @@ -182,6 +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_test_llm(), ) assert "jest" in result["answer"].lower() @@ -197,6 +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_test_llm(), ) assert "python" in result["answer"].lower() @@ -204,6 +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_test_llm(), ) assert "start" in result["answer"].lower() assert "test" in result["answer"].lower() @@ -212,6 +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_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 5b74a8f..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,12 +24,8 @@ 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 + @pytest.fixture(autouse=True) @@ -96,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. " @@ -118,6 +109,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 +155,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 +190,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/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/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 8ed3fb1..a74ed60 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,8 +153,29 @@ 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"] 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/__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..5a39889 --- /dev/null +++ b/tests/unit/agent/llm/test_factory.py @@ -0,0 +1,208 @@ +"""Unit tests for LLM factory.""" + +from unittest.mock import MagicMock, patch + +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 + + +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, + num_retries=3, + 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, + num_retries=3, + 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 + + +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..4add8d3 --- /dev/null +++ b/tests/unit/agent/llm/test_resilience.py @@ -0,0 +1,236 @@ +"""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 unittest.mock as _mock +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, 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.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" + llm_mock.with_structured_output.assert_called_once_with( + _Schema, method="function_calling" + ) + + 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() + 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 + + 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)) + + 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_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.""" diff --git a/tests/unit/agent/tools/test_glob_tool.py b/tests/unit/agent/tools/test_glob_tool.py index 43d3075..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 glob_tool - +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): @@ -9,6 +10,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 +23,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 +33,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 +47,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 +57,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 +68,105 @@ 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"] + + 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"] + + +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 fb1f750..da42f10 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,93 @@ 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_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") + (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"] + + +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 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 5fbfa10..3507ce2 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,76 @@ 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"] + + 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 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")