diff --git a/containers/Containerfile b/containers/Containerfile index bfcb79ba..800d03c1 100644 --- a/containers/Containerfile +++ b/containers/Containerfile @@ -35,8 +35,9 @@ RUN pip install --no-cache-dir \ # Create workspace directory (will be mounted at runtime) RUN mkdir -p /workspace && chmod 777 /workspace -# Copy entrypoint script +# Copy entrypoint and review module COPY entrypoint.py /usr/local/bin/forge-entrypoint.py +COPY review.py /usr/local/bin/review.py RUN chmod +x /usr/local/bin/forge-entrypoint.py # Set working directory diff --git a/containers/entrypoint.py b/containers/entrypoint.py index aeb0aae6..07e0d4a9 100644 --- a/containers/entrypoint.py +++ b/containers/entrypoint.py @@ -19,9 +19,20 @@ import os import subprocess import sys +import time +from datetime import UTC, datetime from pathlib import Path from typing import Any +from review import ( + ReviewCycleData, + Verdict, + detect_review_md, + parse_review_config, + parse_verdict, + write_cycle_file, +) + # Configure logging log_level = os.environ.get("LOG_LEVEL", "INFO").upper() logging.basicConfig( @@ -31,10 +42,12 @@ ) logger = logging.getLogger(__name__) + # Enable LangChain debug/verbose mode if requested if os.environ.get("LANGCHAIN_VERBOSE", "").lower() in ("true", "1", "yes"): try: from langchain_core.globals import set_debug, set_verbose + set_verbose(True) set_debug(True) logger.info("LangChain verbose/debug mode enabled") @@ -224,14 +237,16 @@ def git_commit(workspace: Path, message: str) -> bool: if ls_result.returncode != 0: logger.error(f"git ls-files failed: {ls_result.stderr}") return False - - new_files = [ - f for f in ls_result.stdout.split(b"\0") - if f and not f.startswith(b".forge/") and f != b".forge" - ] + else: + new_files = [ + f + for f in ls_result.stdout.split(b"\0") + if f and not f.startswith(b".forge/") and f != b".forge" + ] if new_files: result = subprocess.run( - ["git", "add", "--"] + [f.decode("utf-8", errors="surrogateescape") for f in new_files], + ["git", "add", "--"] + + [f.decode("utf-8", errors="surrogateescape") for f in new_files], cwd=workspace, capture_output=True, text=True, @@ -349,6 +364,183 @@ def resolve_container_trace_fields(trace_state: dict[str, Any]) -> tuple[list[st return tags, metadata +def _create_llm_model(max_tokens_default: int = 16384): + """Create the LLM model from environment configuration. + + Args: + max_tokens_default: Default max tokens if LLM_MAX_TOKENS is not set. + + Returns: + Tuple of (model_name, model) where model is a LangChain chat model. + + Raises: + RuntimeError: If credentials are missing or Gemini is used without Vertex AI. + """ + model_name = os.environ.get("LLM_MODEL") or os.environ.get( + "CLAUDE_MODEL", "claude-sonnet-4-5@20250929" + ) + api_key = os.environ.get("ANTHROPIC_API_KEY") + vertex_project = os.environ.get("ANTHROPIC_VERTEX_PROJECT_ID") + + if not api_key and not vertex_project: + raise RuntimeError( + "No API credentials found (ANTHROPIC_API_KEY or ANTHROPIC_VERTEX_PROJECT_ID)" + ) + + is_gemini = model_name.lower().startswith(("gemini", "models/gemini")) + max_tokens = int(os.environ.get("LLM_MAX_TOKENS", str(max_tokens_default))) + + if vertex_project: + if is_gemini: + from langchain_google_genai import ChatGoogleGenerativeAI + + logger.info(f"Using Gemini model: {model_name}, max_output_tokens={max_tokens}") + model = ChatGoogleGenerativeAI( + model=model_name, + project=vertex_project, + location=os.environ.get("ANTHROPIC_VERTEX_REGION", "us-east5"), + vertexai=True, + max_output_tokens=max_tokens, + ) + else: + from langchain_google_vertexai.model_garden import ChatAnthropicVertex + + logger.info(f"Using Claude model: {model_name}, max_tokens={max_tokens}") + model = ChatAnthropicVertex( + model_name=model_name, + project=vertex_project, + location=os.environ.get("ANTHROPIC_VERTEX_REGION", "us-east5"), + max_tokens=max_tokens, + ) + else: + if is_gemini: + raise RuntimeError(f"Gemini model '{model_name}' requires Vertex AI credentials") + + from langchain_anthropic import ChatAnthropic + + logger.info(f"Using Claude model: {model_name}, max_tokens={max_tokens}") + model = ChatAnthropic( + model=model_name, + api_key=api_key, + max_tokens=max_tokens, + ) + + return model_name, model + + +def _discover_skill_paths(workspace: Path) -> list[str]: + """Parse AGENT_SKILL_PATHS env var and auto-discover workspace skill dirs. + + Parses comma-separated paths from AGENT_SKILL_PATHS, ensures trailing + slashes, and checks common workspace locations (.claude/skills, + .agents/skills). + + Args: + workspace: Path to the workspace directory. + + Returns: + List of skill directory paths with trailing slashes. + """ + skill_paths: list[str] = [] + skill_paths_env = os.environ.get("AGENT_SKILL_PATHS", "") + if skill_paths_env: + for path in skill_paths_env.split(","): + path = path.strip() + if path: + # Ensure trailing slash for directory paths + if not path.endswith("/"): + path = f"{path}/" + skill_paths.append(path) + + # Auto-discover skill directories in the workspace + # Check common locations for project-specific skills + workspace_skill_dirs = [ + workspace / ".claude" / "skills", + workspace / ".agents" / "skills", + ] + for skill_dir in workspace_skill_dirs: + if skill_dir.is_dir(): + skill_path = f"{skill_dir}/" + if skill_path not in skill_paths: + skill_paths.append(skill_path) + logger.info(f"Auto-discovered workspace skills: {skill_dir}") + + if skill_paths: + logger.info(f"Agent skills: {skill_paths}") + + return skill_paths + + +def _setup_langfuse_tracing(task_key: str, trace_state: dict) -> tuple[dict, bool]: + """Set up Langfuse tracing if credentials are available. + + Imports CallbackHandler, creates handler, and prepares the config dict + with callbacks. The caller must use ``propagate_attributes`` to wrap + the agent invocation when ``langfuse_enabled`` is True. + + Args: + task_key: Jira task key for log context. + trace_state: Workflow fields forwarded to Langfuse (passed through + for the caller to use with ``propagate_attributes``). + + Returns: + Tuple of (config dict with callbacks, langfuse_enabled flag). + """ + _ = trace_state # consumed by caller for propagate_attributes + config: dict = {} + langfuse_enabled = False + if os.environ.get("LANGFUSE_PUBLIC_KEY"): + try: + from langfuse.langchain import CallbackHandler + + handler = CallbackHandler() + config["callbacks"] = [handler] + langfuse_enabled = True + logger.info(f"Langfuse tracing enabled for task {task_key}") + except ImportError: + logger.debug("Langfuse not installed, skipping tracing") + except Exception as e: + logger.warning(f"Failed to initialize Langfuse: {e}") + + return config, langfuse_enabled + + +def _save_conversation_history( + workspace: Path, task_key: str, task_summary: str, result: dict +) -> None: + """Save conversation messages to .forge/history/{task_key}.json. + + Args: + workspace: Path to the workspace directory. + task_key: Jira task key. + task_summary: Short task summary. + result: Agent result dict containing messages. + """ + try: + history_dir = workspace / ".forge" / "history" + history_dir.mkdir(parents=True, exist_ok=True) + history_file = history_dir / f"{task_key}.json" + + # Extract messages from result and serialize + messages = result.get("messages", []) + history_data = { + "task_key": task_key, + "task_summary": task_summary, + "messages": [ + { + "role": getattr(msg, "type", "unknown"), + "content": getattr(msg, "content", str(msg)), + "tool_calls": getattr(msg, "tool_calls", None), + } + for msg in messages + ], + } + history_file.write_text(json.dumps(history_data, indent=2, default=str)) + logger.info(f"Saved conversation history to {history_file}") + except Exception as e: + logger.warning(f"Failed to save conversation history: {e}") + + async def run_agent_task( workspace: Path, task_key: str, @@ -369,28 +561,15 @@ async def run_agent_task( previous_task_keys: List of previously implemented task keys for handoff context. trace_context: Workflow fields forwarded to Langfuse only. """ - # Support both new (LLM_MODEL) and legacy (CLAUDE_MODEL) env var names - model_name = os.environ.get("LLM_MODEL") or os.environ.get("CLAUDE_MODEL", "claude-sonnet-4-5@20250929") logger.info(f"Implementing task: {task_summary}") - logger.info(f"Model: {model_name}") try: from deepagents import create_deep_agent from deepagents.backends import LocalShellBackend - # Check for API credentials - api_key = os.environ.get("ANTHROPIC_API_KEY") - vertex_project = os.environ.get("ANTHROPIC_VERTEX_PROJECT_ID") - - if not api_key and not vertex_project: - logger.error( - "No API credentials found (ANTHROPIC_API_KEY or ANTHROPIC_VERTEX_PROJECT_ID)" - ) - return False + model_name, model = _create_llm_model(max_tokens_default=16384) + logger.info(f"Model: {model_name}") - # Create the agent with local shell backend (enables git commands) - # virtual_mode=False: we want real filesystem access, not virtual paths - # timeout=600: 10 minutes — allows long builds, test suites, and codegen backend = LocalShellBackend( root_dir=str(workspace), inherit_env=True, @@ -408,80 +587,11 @@ async def run_agent_task( "llm_model": model_name, } - # Determine model type (Gemini vs Claude) - is_gemini = model_name.lower().startswith(("gemini", "models/gemini")) - - # Get max tokens from env (default 16384) - max_tokens = int(os.environ.get("LLM_MAX_TOKENS", "16384")) - - if vertex_project: - if is_gemini: - # Gemini models via ChatGoogleGenerativeAI with Vertex AI backend - from langchain_google_genai import ChatGoogleGenerativeAI - - logger.info(f"Using Gemini model: {model_name}, max_output_tokens={max_tokens}") - model = ChatGoogleGenerativeAI( - model=model_name, - project=vertex_project, - location=os.environ.get("ANTHROPIC_VERTEX_REGION", "us-east5"), - vertexai=True, - max_output_tokens=max_tokens, - ) - else: - # Claude models via ChatAnthropicVertex - from langchain_google_vertexai.model_garden import ChatAnthropicVertex - - logger.info(f"Using Claude model: {model_name}, max_tokens={max_tokens}") - model = ChatAnthropicVertex( - model_name=model_name, - project=vertex_project, - location=os.environ.get("ANTHROPIC_VERTEX_REGION", "us-east5"), - max_tokens=max_tokens, - ) - else: - if is_gemini: - logger.error(f"Gemini model '{model_name}' requires Vertex AI credentials") - return False - - from langchain_anthropic import ChatAnthropic - - logger.info(f"Using Claude model: {model_name}, max_tokens={max_tokens}") - model = ChatAnthropic( - model=model_name, - api_key=api_key, - max_tokens=max_tokens, - ) - # Load Context7 MCP tools for library documentation mcp_tools = await load_context7_tools() - # Parse skill paths from environment (comma-separated) - skill_paths = [] - skill_paths_env = os.environ.get("AGENT_SKILL_PATHS", "") - if skill_paths_env: - for path in skill_paths_env.split(","): - path = path.strip() - if path: - # Ensure trailing slash for directory paths - if not path.endswith("/"): - path = f"{path}/" - skill_paths.append(path) - - # Auto-discover skill directories in the workspace - # Check common locations for project-specific skills - workspace_skill_dirs = [ - workspace / ".claude" / "skills", - workspace / ".agents" / "skills", - ] - for skill_dir in workspace_skill_dirs: - if skill_dir.is_dir(): - skill_path = f"{skill_dir}/" - if skill_path not in skill_paths: - skill_paths.append(skill_path) - logger.info(f"Auto-discovered workspace skills: {skill_dir}") - - if skill_paths: - logger.info(f"Agent skills: {skill_paths}") + # Discover skill paths from env and workspace + skill_paths = _discover_skill_paths(workspace) # Create and run the agent. # Note: create_deep_agent already adds SummarizationMiddleware internally — @@ -494,31 +604,17 @@ async def run_agent_task( skills=skill_paths if skill_paths else None, ) - # Set up Langfuse tracing if credentials are available - config: dict = {} - langfuse_enabled = False - if os.environ.get("LANGFUSE_PUBLIC_KEY"): - try: - from langfuse import propagate_attributes - from langfuse.langchain import CallbackHandler - - handler = CallbackHandler() - config["callbacks"] = [handler] - langfuse_enabled = True - logger.info(f"Langfuse tracing enabled for task {task_key}") - except ImportError: - logger.debug("Langfuse not installed, skipping tracing") - except Exception as e: - logger.warning(f"Failed to initialize Langfuse: {e}") + # Set up Langfuse tracing + config, langfuse_enabled = _setup_langfuse_tracing(task_key, trace_state) # Run the agent (with Langfuse session context if enabled) initial_message = { - "messages": [ - {"role": "user", "content": f"Implement this task:\n\n{task_description}"} - ] + "messages": [{"role": "user", "content": f"Implement this task:\n\n{task_description}"}] } if langfuse_enabled: + from langfuse import propagate_attributes + trace_tags, trace_metadata = resolve_container_trace_fields(trace_state) tags = ["forge-container", "task-implementation", *trace_tags] metadata = {"task_summary": task_summary, **trace_metadata} @@ -540,30 +636,8 @@ async def run_agent_task( except Exception: pass - # Save conversation history to .forge/history/{task_key}.json - try: - history_dir = workspace / ".forge" / "history" - history_dir.mkdir(parents=True, exist_ok=True) - history_file = history_dir / f"{task_key}.json" - - # Extract messages from result and serialize - messages = result.get("messages", []) - history_data = { - "task_key": task_key, - "task_summary": task_summary, - "messages": [ - { - "role": getattr(msg, "type", "unknown"), - "content": getattr(msg, "content", str(msg)), - "tool_calls": getattr(msg, "tool_calls", None), - } - for msg in messages - ], - } - history_file.write_text(json.dumps(history_data, indent=2, default=str)) - logger.info(f"Saved conversation history to {history_file}") - except Exception as e: - logger.warning(f"Failed to save conversation history: {e}") + # Save conversation history + _save_conversation_history(workspace, task_key, task_summary, result) logger.info("Agent completed task execution") return True @@ -576,6 +650,306 @@ async def run_agent_task( return False +async def run_reviewer_agent( + workspace: Path, + review_instructions: str, + task_key: str, +) -> str: + """Run reviewer agent with review.md instructions. + + Args: + workspace: Path to the workspace directory. + review_instructions: Instructions from review.md body. + task_key: Jira task key for tracing. + + Returns: + The reviewer agent's output text. + """ + logger.info(f"Running reviewer agent for {task_key}") + + from deepagents import create_deep_agent + from deepagents.backends import LocalShellBackend + + model_name, model = _create_llm_model(max_tokens_default=8192) + logger.info(f"Model: {model_name}") + + backend = LocalShellBackend( + root_dir=str(workspace), + inherit_env=True, + virtual_mode=False, + timeout=300, + ) + + system_prompt = f"""You are a code reviewer agent. Your job is to review the implementation and provide a verdict. + +## Review Instructions +{review_instructions} + +## Verdict Format +After reviewing the code, you MUST output your verdict as either: +- APPROVED - if the implementation meets all requirements +- REJECTED - followed by your feedback if the implementation needs changes + +Example outputs: +- "The implementation looks good. APPROVED" +- "REJECTED: The function is missing error handling. Please add try/except blocks." + +Be specific in your feedback if rejecting.""" + + # Create reviewer agent (no skills needed for review) + agent = create_deep_agent( + model=model, + backend=backend, + system_prompt=system_prompt, + ) + + # Run the reviewer + initial_message = { + "messages": [ + { + "role": "user", + "content": "Please review the implementation in the workspace and provide your verdict.", + } + ] + } + + result = await agent.ainvoke(initial_message) + + # Extract output text from result + messages = result.get("messages", []) + if messages: + last_message = messages[-1] + content = getattr(last_message, "content", str(last_message)) + return content if isinstance(content, str) else str(content) + return "" + + +async def run_worker_with_feedback( + workspace: Path, + task_key: str, + task_summary: str, + task_description: str, + guardrails: str, + feedback: str, + previous_task_keys: list[str] | None = None, + trace_context: dict[str, Any] | None = None, +) -> bool: + """Re-run worker agent with reviewer feedback injected. + + Args: + workspace: Path to the workspace directory. + task_key: Jira task key being implemented. + task_summary: Short task summary. + task_description: Detailed task description. + guardrails: Repository guidelines. + feedback: Reviewer feedback to inject. + previous_task_keys: List of previously implemented task keys. + trace_context: Workflow fields forwarded to Langfuse only. + + Returns: + True if agent completed successfully. + """ + # Inject reviewer feedback section into task description + feedback_section = f"\n\n## Reviewer Feedback\n\n{feedback}\n" + enhanced_description = task_description + feedback_section + + logger.info(f"Re-running worker with feedback for {task_key}") + + return await run_agent_task( + workspace=workspace, + task_key=task_key, + task_summary=task_summary, + task_description=enhanced_description, + guardrails=guardrails, + previous_task_keys=previous_task_keys, + trace_context=trace_context, + ) + + +async def run_review_loop( + workspace: Path, + task_key: str, + task_summary: str, + task_description: str, + guardrails: str, + skill_name: str, + review_md_path: Path, + previous_task_keys: list[str] | None = None, + trace_context: dict[str, Any] | None = None, +) -> bool: + """Run the review loop after initial skill execution. + + Args: + workspace: Path to the workspace directory. + task_key: Jira task key being implemented. + task_summary: Short task summary. + task_description: Detailed task description. + guardrails: Repository guidelines. + skill_name: Name of the skill being reviewed. + review_md_path: Path to the review.md file. + previous_task_keys: List of previously implemented task keys. + trace_context: Workflow fields forwarded to Langfuse only. + + Returns: + True if review loop completed successfully (approved or max retries). + """ + # Parse review configuration (SC-004, SC-005) + config = parse_review_config(review_md_path) + max_retries = config.max_retries + instructions = config.instructions + + logger.info(f"Starting review loop for {skill_name} (max_retries={max_retries})") + logger.info( + f"Review instructions: {instructions[:200]}..." + if len(instructions) > 200 + else f"Review instructions: {instructions}" + ) + + if max_retries == 0: + logger.info(f"Review disabled (max_retries=0) for {skill_name}") + return True + + cycle = 0 + while cycle < max_retries: + cycle += 1 + cycle_start = time.perf_counter() + logger.info(f"Review cycle {cycle}/{max_retries}") + + # Run reviewer agent (SC-001) + try: + reviewer_output = await run_reviewer_agent( + workspace=workspace, + review_instructions=instructions, + task_key=task_key, + ) + except Exception as e: + logger.error(f"Reviewer agent failed: {e}") + # Treat reviewer failure as rejection + reviewer_output = f"REJECTED: Reviewer agent error: {e}" + + # Parse verdict (SC-002) + verdict, feedback = parse_verdict(reviewer_output) + elapsed = time.perf_counter() - cycle_start + timestamp = datetime.now(UTC).strftime("%Y-%m-%dT%H:%M:%SZ") + + logger.info(f"Review verdict: {verdict}, elapsed: {elapsed:.2f}s") + if feedback: + logger.info( + f"Feedback: {feedback[:200]}..." if len(feedback) > 200 else f"Feedback: {feedback}" + ) + + # Write cycle file (SC-007) + cycle_data = ReviewCycleData( + cycle=cycle, + max_cycles=max_retries, + verdict=verdict, + feedback=feedback, + skill=skill_name, + elapsed_seconds=elapsed, + timestamp=timestamp, + ) + write_cycle_file(workspace, task_key, skill_name, cycle_data) + + # On APPROVED: Exit loop successfully (SC-002) + if verdict == Verdict.APPROVED: + logger.info(f"Review approved on cycle {cycle}") + return True + + # On REJECTED with retries remaining (SC-003) + if cycle < max_retries: + logger.info(f"Retrying with feedback (attempt {cycle + 1}/{max_retries})") + worker_success = await run_worker_with_feedback( + workspace=workspace, + task_key=task_key, + task_summary=task_summary, + task_description=task_description, + guardrails=guardrails, + feedback=feedback, + previous_task_keys=previous_task_keys, + trace_context=trace_context, + ) + if not worker_success: + logger.error(f"Worker retry failed on cycle {cycle}") + break + + # Max retries exhausted - exit with success (BR-005) + if max_retries > 0: + logger.warning(f"Review loop exhausted max_retries ({max_retries}) for {skill_name}") + return True + + +def _parse_task_config(args) -> dict: + """Parse task configuration from task file or CLI args. + + Returns: + Dict with keys: task_key, task_summary, task_description, + skill_name, previous_task_keys, trace_context. + + Raises: + SystemExit: With EXIT_CONFIG_ERROR on missing/invalid config. + """ + previous_task_keys: list[str] = [] + trace_context: dict[str, Any] = {} + task_key: str = "UNKNOWN" + if args.task_file: + if not args.task_file.exists(): + logger.error(f"Task file not found: {args.task_file}") + sys.exit(EXIT_CONFIG_ERROR) + + task_data = json.loads(args.task_file.read_text()) + task_key = task_data.get("task_key", "UNKNOWN") + task_summary = task_data.get("summary", "") + task_description = task_data.get("description", "") + previous_task_keys = task_data.get("previous_task_keys", []) + raw_trace_context = task_data.get("trace_context", {}) + trace_context = raw_trace_context if isinstance(raw_trace_context, dict) else {} + skill_name = task_data.get("skill_name", "") + elif args.task_summary and args.task_description: + task_summary = args.task_summary + task_description = args.task_description + skill_name = "" + else: + logger.error( + "Task details required: use --task-file or --task-summary + --task-description" + ) + sys.exit(EXIT_CONFIG_ERROR) + + return { + "task_key": task_key, + "task_summary": task_summary, + "task_description": task_description, + "skill_name": skill_name, + "previous_task_keys": previous_task_keys, + "trace_context": trace_context, + } + + +def _fallback_commit(workspace: Path, task_key: str, task_summary: str) -> None: + """Check if workspace is a git repo and run git_commit() as fallback. + + Skips if workspace is not a git repo -- analysis tasks (RCA, reflection) + write artifacts to .forge/ without needing a commit. + + Raises: + SystemExit: With EXIT_TASK_FAILED if commit fails. + """ + is_git_repo = ( + subprocess.run( + ["git", "rev-parse", "--is-inside-work-tree"], + cwd=workspace, + capture_output=True, + ).returncode + == 0 + ) + if is_git_repo: + fallback_message = ( + f"[{task_key}] {task_summary}\n\nAuto-committed by Forge container fallback." + ) + if not git_commit(workspace, fallback_message): + logger.error("Failed to commit changes") + sys.exit(EXIT_TASK_FAILED) + + def main(): parser = argparse.ArgumentParser( description="Forge container entrypoint for AI code implementation" @@ -607,36 +981,24 @@ def main(): args = parser.parse_args() - # Load task details - previous_task_keys: list[str] = [] - trace_context: dict[str, Any] = {} - task_key: str = "UNKNOWN" - if args.task_file: - if not args.task_file.exists(): - logger.error(f"Task file not found: {args.task_file}") - sys.exit(EXIT_CONFIG_ERROR) - - task_data = json.loads(args.task_file.read_text()) - task_key = task_data.get("task_key", "UNKNOWN") - task_summary = task_data.get("summary", "") - task_description = task_data.get("description", "") - previous_task_keys = task_data.get("previous_task_keys", []) - raw_trace_context = task_data.get("trace_context", {}) - trace_context = raw_trace_context if isinstance(raw_trace_context, dict) else {} - elif args.task_summary and args.task_description: - task_summary = args.task_summary - task_description = args.task_description - else: - logger.error( - "Task details required: use --task-file or --task-summary + --task-description" - ) - sys.exit(EXIT_CONFIG_ERROR) + # Parse task configuration + task_config = _parse_task_config(args) + task_key = task_config["task_key"] + task_summary = task_config["task_summary"] + task_description = task_config["task_description"] + skill_name = task_config["skill_name"] + previous_task_keys = task_config["previous_task_keys"] + trace_context = task_config["trace_context"] workspace = args.workspace if not workspace.exists(): logger.error(f"Workspace not found: {workspace}") sys.exit(EXIT_CONFIG_ERROR) + # Get skill_name from env var if not in task data + if not skill_name: + skill_name = os.environ.get("FORGE_SKILL_NAME", "") + # Configure git for commits configure_git() @@ -672,19 +1034,33 @@ def main(): logger.error("Task implementation failed") sys.exit(EXIT_TASK_FAILED) - # Ensure changes are committed (agent should have done this, but as fallback). - # Skip if workspace is not a git repo — analysis tasks (RCA, reflection) write - # artifacts to .forge/ without needing a commit. - is_git_repo = subprocess.run( - ["git", "rev-parse", "--is-inside-work-tree"], - cwd=workspace, - capture_output=True, - ).returncode == 0 - if is_git_repo: - fallback_message = f"[{task_key}] {task_summary}\n\nAuto-committed by Forge container fallback." - if not git_commit(workspace, fallback_message): - logger.error("Failed to commit changes") - sys.exit(EXIT_TASK_FAILED) + # Check for review.md and run review loop if it exists (SC-001, SC-010) + if skill_name: + review_md_path = detect_review_md(skill_name) + + if review_md_path: + logger.info(f"Found review.md at {review_md_path}, starting review loop") + # run_review_loop always returns True (exhaustion proceeds to PR per BR-005) + asyncio.run( + run_review_loop( + workspace=workspace, + task_key=task_key, + task_summary=task_summary, + task_description=task_description, + guardrails=guardrails, + skill_name=skill_name, + review_md_path=review_md_path, + previous_task_keys=previous_task_keys, + trace_context=trace_context, + ) + ) + else: + logger.info(f"No review.md found for skill {skill_name}, skipping review loop") + else: + logger.info("No skill_name provided, skipping review loop") + + # Ensure changes are committed (agent should have done this, but as fallback) + _fallback_commit(workspace, task_key, task_summary) logger.info("Task completed successfully") sys.exit(EXIT_SUCCESS) diff --git a/containers/review.py b/containers/review.py new file mode 100644 index 00000000..c1524a9f --- /dev/null +++ b/containers/review.py @@ -0,0 +1,328 @@ +"""Review loop data structures for the container review engine. + +This module provides core data structures used by the review loop engine: + +- ReviewConfig: Configuration parsed from review.md frontmatter +- ReviewCycleData: Data captured for each review cycle iteration +- Verdict: Enum for review outcomes (APPROVED/REJECTED) +- parse_review_config: Parser for review.md YAML frontmatter +- detect_review_md: Locates review.md with project override precedence +- parse_verdict: Extracts verdict and feedback from review output text +- write_cycle_file: Writes review cycle data to JSON file +""" + +import json +import logging +import os +import re +from dataclasses import asdict, dataclass +from enum import StrEnum +from pathlib import Path + +import yaml + +logger = logging.getLogger(__name__) + +# Default max retries for review loops +DEFAULT_MAX_RETRIES = 3 + +# Environment variable for max retries override +ENV_MAX_RETRIES = "AUTO_REVIEW_MAX_RETRIES" + + +class Verdict(StrEnum): + """Review verdict indicating approval or rejection.""" + + APPROVED = "approved" + REJECTED = "rejected" + + +@dataclass +class ReviewConfig: + """Configuration for review loops parsed from review.md frontmatter. + + Attributes: + max_retries: Maximum number of retry attempts (default: 3). + instructions: Review instructions from the markdown body. + """ + + max_retries: int = DEFAULT_MAX_RETRIES + instructions: str = "" + + +@dataclass +class ReviewCycleData: + """Data captured for a single review cycle iteration. + + Attributes: + cycle: Current cycle number (1-indexed). + max_cycles: Maximum cycles allowed. + verdict: Review outcome ("approved" or "rejected"). + feedback: Reviewer feedback text. + skill: Name of the skill that performed the review. + elapsed_seconds: Time taken for this review cycle. + timestamp: ISO 8601 UTC timestamp of cycle completion. + """ + + cycle: int + max_cycles: int + verdict: str + feedback: str + skill: str + elapsed_seconds: float + timestamp: str + + +# Regex pattern for YAML frontmatter: --- at start, optional content, --- delimiter +# The frontmatter content between delimiters may be empty (just newlines) +_FRONTMATTER_PATTERN = re.compile( + r"\A---\s*\n(.*?)---\s*\n?(.*)", + re.DOTALL, +) + + +def parse_review_config(review_md_path: Path) -> ReviewConfig: + """Parse ReviewConfig from a review.md file with YAML frontmatter. + + The file format is: + ``` + --- + max_retries: 5 + --- + Review instructions here... + ``` + + Priority for max_retries: + 1. YAML frontmatter value + 2. AUTO_REVIEW_MAX_RETRIES environment variable + 3. Default value (3) + + If the file does not exist or YAML parsing fails, returns defaults + with a warning log (per BR-006). + + Args: + review_md_path: Path to the review.md file. + + Returns: + ReviewConfig with parsed values or defaults. + """ + # Get env var fallback for max_retries + env_max_retries: int | None = None + env_value = os.environ.get(ENV_MAX_RETRIES) + if env_value is not None: + try: + env_max_retries = max(0, int(env_value)) + except ValueError: + logger.warning( + "Invalid %s value %r, ignoring", + ENV_MAX_RETRIES, + env_value, + ) + + # Default config to return on errors + default_max_retries = env_max_retries if env_max_retries is not None else DEFAULT_MAX_RETRIES + + if not review_md_path.exists(): + logger.warning("Review config not found at %s, using defaults", review_md_path) + return ReviewConfig(max_retries=default_max_retries, instructions="") + + try: + content = review_md_path.read_text(encoding="utf-8") + except OSError as e: + logger.warning("Failed to read %s: %s, using defaults", review_md_path, e) + return ReviewConfig(max_retries=default_max_retries, instructions="") + + # Try to parse frontmatter + match = _FRONTMATTER_PATTERN.match(content) + if not match: + # No frontmatter found - treat entire content as instructions + return ReviewConfig(max_retries=default_max_retries, instructions=content.strip()) + + frontmatter_raw = match.group(1) + instructions = match.group(2).strip() + + # Parse YAML frontmatter + try: + frontmatter = yaml.safe_load(frontmatter_raw) + except yaml.YAMLError as e: + logger.warning( + "Malformed YAML frontmatter in %s: %s, using defaults", + review_md_path, + e, + ) + return ReviewConfig(max_retries=default_max_retries, instructions=instructions) + + # Handle empty frontmatter + if frontmatter is None: + frontmatter = {} + + # Extract max_retries with fallback chain + max_retries = default_max_retries + if isinstance(frontmatter, dict) and "max_retries" in frontmatter: + fm_value = frontmatter["max_retries"] + if isinstance(fm_value, int) and not isinstance(fm_value, bool): + max_retries = max(0, fm_value) + else: + logger.warning( + "Invalid max_retries value %r in %s, using default", + fm_value, + review_md_path, + ) + + return ReviewConfig(max_retries=max_retries, instructions=instructions) + + +def find_skill_file( + skill_name: str, + filename: str, + skill_paths: list[str] | None = None, +) -> Path | None: + """Find a file within a skill directory across all mounted skill paths. + + Searches each skill path for {skill_name}/{filename}. Later paths in the + list take precedence (matching the convention where project-specific skill + directories are mounted after defaults). + + The skill paths are read from the AGENT_SKILL_PATHS environment variable + if not provided explicitly. + + Args: + skill_name: Name of the skill (e.g., "implement-task"). + filename: File to find (e.g., "SKILL.md", "review.md"). + skill_paths: List of skill directory paths. If None, reads from + AGENT_SKILL_PATHS env var. + + Returns: + Path to the file if found, None otherwise. + """ + if skill_paths is None: + env_val = os.environ.get("AGENT_SKILL_PATHS", "") + skill_paths = [p.strip() for p in env_val.split(",") if p.strip()] + + found: Path | None = None + for base in skill_paths: + candidate = Path(base) / skill_name / filename + if candidate.is_file(): + logger.debug("Found %s at %s", filename, candidate) + found = candidate + + if found is None: + logger.debug( + "No %s found for skill %r in paths: %s", + filename, + skill_name, + skill_paths, + ) + + return found + + +def detect_review_md( + skill_name: str, + skill_paths: list[str] | None = None, +) -> Path | None: + """Detect the review.md file for a skill. + + Delegates to find_skill_file, searching all mounted skill paths. + + Args: + skill_name: Name of the skill (e.g., "implement-task"). + skill_paths: Explicit skill paths to search. + + Returns: + Path to review.md if found, None otherwise. + """ + return find_skill_file(skill_name, "review.md", skill_paths) + + +def parse_verdict(output_text: str) -> tuple[Verdict, str]: + """Extract verdict and feedback from review output text. + + Performs case-insensitive search for "APPROVED" and "REJECTED" markers. + When both markers are present, the first occurrence wins (checks APPROVED first). + + Args: + output_text: The raw output text from the review process. + + Returns: + Tuple of (Verdict, feedback_string): + - (Verdict.APPROVED, "") when APPROVED marker is found first + - (Verdict.REJECTED, feedback) when REJECTED marker is found first, + where feedback is the text following the marker + - (Verdict.REJECTED, "Verdict could not be parsed") when neither marker found + """ + # Search original text with IGNORECASE to avoid Unicode length mismatch + # (str.upper() can change length for certain chars like ß→SS) + approved_match = re.search(r"\bAPPROVED\b", output_text, re.IGNORECASE) + rejected_match = re.search(r"\bREJECTED\b", output_text, re.IGNORECASE) + approved_pos = approved_match.start() if approved_match else -1 + rejected_pos = rejected_match.start() if rejected_match else -1 + + if approved_pos == -1 and rejected_pos == -1: + return (Verdict.REJECTED, "Verdict could not be parsed") + + if approved_pos != -1 and (rejected_pos == -1 or approved_pos < rejected_pos): + return (Verdict.APPROVED, "") + + feedback_start = rejected_match.end() + feedback = output_text[feedback_start:].strip() + + return (Verdict.REJECTED, feedback) + + +def review_cycle_dir_name(task_key: str, skill_name: str) -> str: + """Build the directory name for review cycle files. + + Format: {task_key}__{skill_name} (double underscore separator). + + Args: + task_key: Jira task key (e.g., "AISOS-2126"). + skill_name: Skill name (e.g., "implement-task"). + + Returns: + Directory name string. + """ + return f"{task_key}__{skill_name}" + + +def write_cycle_file( + workspace: Path, task_key: str, skill_name: str, cycle_data: ReviewCycleData +) -> None: + """Write review cycle data to a JSON file. + + Creates .forge/reviews/{task_key}__{skill_name}/ and writes + review_cycle_N.json where N is the cycle number. + + Args: + workspace: Path to the workspace root directory. + task_key: Jira task key (e.g., "AISOS-2126"). + skill_name: Skill name (e.g., "implement-task"). + cycle_data: ReviewCycleData instance to serialize. + """ + dir_name = review_cycle_dir_name(task_key, skill_name) + output_dir = workspace / ".forge" / "reviews" / dir_name + output_dir.mkdir(parents=True, exist_ok=True) + + output_file = output_dir / f"review_cycle_{cycle_data.cycle}.json" + + # Convert dataclass to dict + data = asdict(cycle_data) + + # Ensure verdict is lowercase string (handle Verdict enum or string) + verdict_value = data["verdict"] + if hasattr(verdict_value, "value"): + # It's a Verdict enum + data["verdict"] = verdict_value.value + else: + # It's already a string, ensure lowercase + data["verdict"] = str(verdict_value).lower() + + # Atomic write: write to temp file then rename to avoid partial reads + tmp_file = output_file.with_suffix(".tmp") + tmp_file.write_text( + json.dumps(data, indent=2, ensure_ascii=False) + "\n", + encoding="utf-8", + ) + tmp_file.rename(output_file) + + logger.debug("Wrote review cycle file: %s", output_file) diff --git a/docs/guide/auto-review.md b/docs/guide/auto-review.md new file mode 100644 index 00000000..eb8b8df0 --- /dev/null +++ b/docs/guide/auto-review.md @@ -0,0 +1,419 @@ +# Auto-Review + +Auto-review is a quality gate that runs after skill execution. It spawns a reviewer agent to evaluate the skill's output and triggers retries with feedback when the result doesn't meet quality criteria. + +## Overview + +When a skill has a `review.md` file in its directory, Forge automatically invokes a reviewer agent after the skill completes. The reviewer evaluates the output according to instructions in `review.md` and issues a verdict: + +- **APPROVED** — The output meets quality criteria; workflow continues +- **REJECTED** — The output has issues; skill re-runs with feedback injected + +```mermaid +flowchart LR + A[Skill Executes] --> B{review.md exists?} + B -->|No| C[Continue Workflow] + B -->|Yes| D[Spawn Reviewer Agent] + D --> E{Verdict?} + E -->|APPROVED| C + E -->|REJECTED| F{Retries left?} + F -->|Yes| G[Re-run Skill with Feedback] + G --> D + F -->|No| C +``` + +### When to Use Auto-Review + +Auto-review is useful when: + +- A skill produces artifacts that need quality validation before proceeding +- You want to catch common mistakes (missing tests, debug code, documentation gaps) without human intervention +- You need consistent quality standards across all executions of a skill + +Auto-review is **not** a replacement for human code review — it catches machine-detectable issues early in the pipeline. + +## Quick Start + +Create a `review.md` file in your skill directory: + +``` +skills/ +└── default/ + └── implement-task/ + ├── SKILL.md + └── review.md ← Reviewer configuration +``` + +Minimal `review.md`: + +```markdown +--- +max_retries: 3 +--- + +# Quality Check + +Verify the implementation: + +1. All new public functions have tests +2. No debug print statements in committed code +3. Error handling uses specific exception types + +Output APPROVED if all criteria pass, or REJECTED with specific feedback. +``` + +That's it. The next time `implement-task` runs, the reviewer will evaluate its output. + +## Configuration + +### `max_retries` in Frontmatter + +The `max_retries` field in YAML frontmatter controls how many times a skill can retry after a REJECTED verdict: + +```markdown +--- +max_retries: 5 +--- + +Review instructions here... +``` + +### `AUTO_REVIEW_MAX_RETRIES` Environment Variable + +Set a global default for all skills that don't specify `max_retries` in their `review.md`: + +```bash +# In .env or environment +AUTO_REVIEW_MAX_RETRIES=5 +``` + +### Priority Chain + +When determining retry limit, Forge checks in order: + +| Priority | Source | Example | +|----------|--------|---------| +| 1 (highest) | Frontmatter `max_retries` | `max_retries: 5` in review.md | +| 2 | Environment variable | `AUTO_REVIEW_MAX_RETRIES=5` | +| 3 (lowest) | Built-in default | `3` | + +### Default Value + +If neither frontmatter nor environment variable is set, the default `max_retries` is **3**. + +## Writing Review Instructions + +The prose body of `review.md` (everything after the `---` closing delimiter) becomes the reviewer agent's instructions. Write clear, actionable criteria. + +### Effective Review Instructions + +**Do:** + +- Define explicit pass/fail criteria for each quality dimension +- Include examples of acceptable vs. flagged patterns +- Specify the expected output format (APPROVED/REJECTED markers) +- Reference specific files or artifacts the reviewer should examine + +**Don't:** + +- Write vague instructions ("make sure the code is good") +- Include implementation details unrelated to review +- Duplicate instructions from the skill's SKILL.md + +### Example: Comprehensive Review + +```markdown +--- +max_retries: 3 +--- + +# Implementation Review + +Review code changes via `git diff origin/main...HEAD`. + +## Criteria + +### Test Coverage +- Public functions must have tests +- Flag: new public function without corresponding test + +### Error Handling +- No bare `except:` or empty `catch {}` +- Flag: generic error swallowing + +### Documentation +- Public APIs need docstrings +- Flag: public function without documentation + +## Verdict + +Output exactly one: +- `APPROVED` — all criteria pass +- `REJECTED` — followed by specific, actionable feedback +``` + +See [skills/default/implement-task/review.md](https://github.com/your-org/forge/blob/main/skills/default/implement-task/review.md) for a complete example. + +## Verdict Protocol + +The reviewer must output exactly one verdict marker. Forge scans the output text for these markers. + +### APPROVED + +Use when all criteria pass: + +``` +APPROVED +``` + +Or with optional notes: + +``` +APPROVED + +Notes: +- Consider adding edge case tests (non-blocking) +``` + +### REJECTED + +Use when any criterion fails. Always include specific, actionable feedback: + +``` +REJECTED + +Issues: +- [Test Coverage] src/handler.py:42 — `process_data()` is public but has no test +- [Error Handling] pkg/db.go:87 — error from `Query()` discarded with `_` + +Required changes: +1. Add test for `process_data()` in `test_handler.py` +2. Handle or propagate error from `Query()` at line 87 +``` + +### Verdict Detection + +Forge performs case-insensitive substring matching for `APPROVED` and `REJECTED`: + +- First marker found wins (if both appear, the one occurring first determines verdict) +- Marker can appear anywhere in the output (start, middle, end) +- Surrounding text is captured as feedback for REJECTED verdicts + +### No Verdict Found + +If neither marker appears, Forge treats the output as **REJECTED** with the feedback: + +``` +Verdict could not be parsed +``` + +This prevents silent failures from malformed reviewer output. + +## Per-Project Overrides + +Review configuration follows the same precedence rules as skill files. See [Authoring Skills](../skills/authoring.md) for the general pattern. + +### Resolution Order + +For a ticket `MYPROJECT-123` running skill `implement-task`: + +| Priority | Path | Description | +|----------|------|-------------| +| 1 (highest) | `skills/myproject/implement-task/review.md` | Project override | +| 2 | `skills/default/implement-task/review.md` | Default configuration | + +### Override Behavior + +- If the project override exists, it **completely replaces** the default (no merging) +- If neither exists, the skill runs **without a reviewer** +- Project key is extracted from ticket key and lowercased (`MYPROJECT-123` → `myproject`) + +### Example: Project-Specific Review + +``` +skills/ +├── default/ +│ └── implement-task/ +│ └── review.md ← Generic criteria for all projects +└── myproject/ + └── implement-task/ + └── review.md ← Stricter criteria for myproject +``` + +The `myproject` override might add additional criteria: + +```markdown +--- +max_retries: 5 +--- + +# MyProject Code Review + +All default criteria plus: + +## MyProject-Specific + +### Performance +- No N+1 database queries +- Bulk operations must use batching + +### Security +- All user input must be validated +- No raw SQL queries (use ORM) +``` + +## Observability + +### Review Cycle Files + +Each review cycle writes a JSON file to the workspace: + +``` +.forge/{step-name}/review_cycle_N.json +``` + +Where: +- `{step-name}` is the workflow step (e.g., `implement_task`, `local_review`) +- `N` is the cycle number (1-indexed) + +**Example path:** `.forge/implement_task/review_cycle_1.json` + +### File Contents + +```json +{ + "cycle": 1, + "max_cycles": 3, + "verdict": "rejected", + "feedback": "Missing tests for process_data() function", + "skill": "implement-task", + "elapsed_seconds": 12.5, + "timestamp": "2024-01-15T10:30:00Z" +} +``` + +| Field | Type | Description | +|-------|------|-------------| +| `cycle` | int | Current cycle number (1-indexed) | +| `max_cycles` | int | Maximum allowed cycles | +| `verdict` | string | `"approved"` or `"rejected"` | +| `feedback` | string | Reviewer feedback text (empty for approved) | +| `skill` | string | Name of the skill being reviewed | +| `elapsed_seconds` | float | Time spent on this review cycle | +| `timestamp` | string | ISO 8601 UTC timestamp | + +### Prometheus Metrics + +Forge exposes these metrics at `/metrics`: + +| Metric | Type | Labels | Description | +|--------|------|--------|-------------| +| `forge_review_cycles_total` | Counter | `skill`, `step` | Total review cycles detected | +| `forge_review_verdicts_total` | Counter | `skill`, `step`, `verdict` | Verdicts by outcome | +| `forge_review_duration_seconds` | Histogram | `skill`, `step` | Review cycle duration | + +### Terminal Progress + +In local/terminal mode, Forge prints progress during review loops: + +``` +Review cycle 1/3: REJECTED - Missing tests for process_data() function... +Review cycle 2/3: REJECTED - Tests added but error handling still missing... +Review cycle 3/3: APPROVED +``` + +Feedback is truncated to 200 characters in terminal output. + +## Troubleshooting + +### Reviewer Times Out + +**Symptom:** Review cycle takes > 5 minutes and fails. + +**Causes:** +- Reviewer instructions are too complex or ambiguous +- Large codebase with many files to examine +- Reviewer agent loops or gets stuck + +**Solutions:** +- Simplify review criteria to focus on specific, checkable items +- Use `git diff` instead of reading entire files +- Add explicit instructions to limit scope + +### Verdict Parsing Failures + +**Symptom:** Every cycle shows "Verdict could not be parsed" even though reviewer output looks correct. + +**Causes:** +- Marker is misspelled (`APROVED`, `REJECT`) +- Marker appears inside a code block (not detected) +- Non-ASCII characters in marker + +**Solutions:** +- Ensure reviewer outputs exactly `APPROVED` or `REJECTED` +- Add explicit instructions: "Output the word APPROVED or REJECTED on its own line" +- Check reviewer output in cycle JSON files + +### Endless Loops (Max Retries Hit) + +**Symptom:** Skill always exhausts `max_retries` without passing. + +**Causes:** +- Review criteria are impossible to satisfy automatically +- Feedback is not actionable by the skill +- Skill doesn't understand the feedback format + +**Solutions:** +- Review the feedback in cycle JSON files — is it specific enough? +- Simplify criteria to things the skill can actually fix +- Increase `max_retries` if issues are intermittent +- Consider whether the criteria should cause rejection at all + +### review.md Not Detected + +**Symptom:** Skill runs without review even though `review.md` exists. + +**Causes:** +- File is in wrong directory or has wrong name +- File is named `Review.md` (wrong case) +- Project override path is incorrect + +**Solutions:** +- Verify exact path: `skills/{project}/{skill-name}/review.md` +- Check file is named `review.md` (lowercase) +- Verify project key extraction (`PROJ-123` → `proj`) + +### YAML Frontmatter Errors + +**Symptom:** `max_retries` setting is ignored. + +**Causes:** +- Frontmatter is malformed YAML +- `max_retries` value is not an integer +- Missing `---` delimiters + +**Solutions:** +- Validate YAML syntax +- Use integer value: `max_retries: 3` (not `"3"` or `three`) +- Ensure file starts with `---` on first line +- Check worker logs for "Malformed YAML" warnings + +### Feedback Not Injected on Retry + +**Symptom:** Skill re-runs but doesn't see previous feedback. + +**Causes:** +- Conversation history file missing or corrupted +- Feedback format not recognized by skill + +**Solutions:** +- Check `.forge/history/{task_key}.json` exists +- Verify "## Reviewer Feedback" section appears in retry prompt +- Review skill's handling of feedback sections + +## Related Documentation + +- [Authoring Skills](../skills/authoring.md) — General skill authoring guide +- [review.md Schema](../reference/review-md-schema.md) — Complete file format specification +- [Feature Workflow](feature-workflow.md) — How auto-review fits in the feature pipeline +- [Bug Workflow](bug-workflow.md) — How auto-review fits in the bug pipeline diff --git a/docs/reference/config.md b/docs/reference/config.md index 1f8bbecb..2a890cd9 100644 --- a/docs/reference/config.md +++ b/docs/reference/config.md @@ -106,6 +106,16 @@ Use these to skip the Jira project property requirement during local development | `CONTAINER_MEMORY_LIMIT` | Memory limit for task containers (default: `4g`) | | `CONTAINER_CPU_LIMIT` | CPU limit for task containers (default: `2`) | +## Auto-Review + +Settings for the automatic review loop that runs after skill execution. See the [Auto-Review Guide](../guide/auto-review.md) for details. + +| Variable | Default | Description | +|----------|---------|-------------| +| `AUTO_REVIEW_MAX_RETRIES` | `3` | Default maximum retry attempts when a skill's `review.md` doesn't specify `max_retries` | +| `AUTO_REVIEW_POLL_INTERVAL` | `5.0` | Polling interval in seconds for detecting review cycle files during container execution | +| `AUTO_REVIEW_RECORD_POLLED_FILES` | (none) | Recording mode for polled review cycle files: `log` logs cycle data at INFO level, `copy` copies files to recording directory | + ## Observability ### Langfuse Tracing diff --git a/docs/reference/review-md-schema.md b/docs/reference/review-md-schema.md new file mode 100644 index 00000000..da8dc45b --- /dev/null +++ b/docs/reference/review-md-schema.md @@ -0,0 +1,230 @@ +# review.md Schema + +This document specifies the file format and validation rules for `review.md` files used to configure skill-specific review loops. + +## Overview + +A `review.md` file configures how an AI reviewer evaluates the output of a skill before proceeding. It specifies retry limits and provides review instructions that guide the reviewer's analysis. + +## File Location + +### Naming Convention + +The file must be named exactly `review.md` (lowercase) and placed alongside `SKILL.md` in a skill directory: + +``` +skills/ +├── default/ +│ └── local-code-review/ +│ ├── SKILL.md # Skill definition +│ └── review.md # Review configuration +└── myproject/ + └── local-code-review/ + ├── SKILL.md # Project-specific skill override + └── review.md # Project-specific review override +``` + +### Override Precedence + +Review configuration follows the same precedence rules as skills: + +| Priority | Path | Description | +|----------|------|-------------| +| 1 (highest) | `skills/{project}/{skill-name}/review.md` | Project-specific override | +| 2 | `skills/default/{skill-name}/review.md` | Default configuration | + +The project key is extracted from the Jira ticket key and lowercased. For ticket `MYPROJECT-123`, Forge checks `skills/myproject/` first. + +**Examples:** + +- Ticket `AISOS-456` with skill `local-code-review`: + 1. Check `skills/aisos/local-code-review/review.md` + 2. Fall back to `skills/default/local-code-review/review.md` + +- If neither path exists, the skill runs **without a reviewer**. + +## File Format + +The `review.md` file uses YAML frontmatter followed by Markdown prose, consistent with `SKILL.md` files: + +```markdown +--- +max_retries: 5 +--- + +# Review Instructions + +Verify that the implementation meets all acceptance criteria... +``` + +### YAML Frontmatter Schema + +The frontmatter section is delimited by `---` markers and contains YAML key-value pairs: + +| Field | Type | Required | Default | Description | +|-------|------|----------|---------|-------------| +| `max_retries` | `int` | No | `3` | Maximum retry attempts after a REJECTED verdict. Set to `0` to skip review. | + +**Disabling review:** Set `max_retries: 0` in the skill's `review.md` frontmatter, or set the `AUTO_REVIEW_MAX_RETRIES=0` environment variable to disable review globally for all skills. + +**Frontmatter rules:** + +- The opening `---` must be the first line of the file (no leading whitespace or blank lines) +- The closing `---` must appear on its own line +- Unknown fields are silently ignored (forward compatibility) +- Field values must match the expected type; invalid types trigger a warning and use defaults + +**Priority for `max_retries`:** + +1. Frontmatter value (if valid integer) +2. `AUTO_REVIEW_MAX_RETRIES` environment variable (if set and valid) +3. Built-in default: `3` + +### Prose Body + +Everything after the closing `---` delimiter is treated as reviewer instructions. This Markdown content is passed to the reviewer agent as its system prompt. + +**Best practices for instructions:** + +- Define what constitutes approval vs. rejection +- Specify quality criteria and acceptance thresholds +- Reference any artifacts the reviewer should examine (e.g., `.forge/` files, test output) +- Keep instructions focused on review criteria, not implementation details + +**Example:** + +```markdown +--- +max_retries: 3 +--- + +# Code Review Checklist + +Evaluate the implementation against these criteria: + +## Required (reject if missing) +- [ ] All acceptance criteria from the task are addressed +- [ ] No obvious security vulnerabilities introduced +- [ ] Tests cover the new functionality + +## Recommended (note but don't reject) +- [ ] Code follows existing patterns +- [ ] Documentation updated if needed + +## Output Format + +End your review with exactly one of: +- `APPROVED` — all required criteria pass +- `REJECTED` — followed by specific feedback for the implementer +``` + +## Edge Cases + +### Empty File + +A `review.md` file with zero bytes or only whitespace: + +- Reviewer spawns with **no instructions** (empty prompt) +- Uses default `max_retries: 3` (or env var if set) + +### Frontmatter-Only File + +A file with frontmatter but no prose body: + +```markdown +--- +max_retries: 5 +--- +``` + +- Reviewer spawns with **no instructions** (empty prompt) +- Uses the specified `max_retries` value + +### Missing Frontmatter + +A file without `---` delimiters: + +```markdown +Review the code for bugs and style issues. +``` + +- Entire file content becomes reviewer instructions +- Uses default `max_retries: 3` (or env var if set) + +### Malformed YAML + +If the frontmatter contains invalid YAML: + +```markdown +--- +max_retries: "not a number" +--- + +Instructions here... +``` + +- A warning is logged +- Default `max_retries` is used +- Prose body is still extracted and used as instructions + +### No review.md Found + +If no `review.md` exists in either the project or default skill directory: + +- **No reviewer is spawned** — the skill output is accepted without review +- This is the expected behavior for skills that don't require review + +## Review Cycle Output + +Each review cycle writes its results to: + +``` +.forge/{step-name}/review_cycle_N.json +``` + +Where: +- `{step-name}` is the workflow step (e.g., `implement_task`, `local_code_review`) +- `N` is the 1-indexed cycle number + +**Example:** `.forge/implement_task/review_cycle_1.json` + +**JSON schema:** + +```json +{ + "cycle": 1, + "max_cycles": 3, + "verdict": "rejected", + "feedback": "Missing test coverage for edge case...", + "skill": "local-code-review", + "elapsed_seconds": 45.2, + "timestamp": "2024-01-15T10:30:00Z" +} +``` + +| Field | Type | Description | +|-------|------|-------------| +| `cycle` | `int` | Current cycle number (1-indexed) | +| `max_cycles` | `int` | Maximum cycles allowed (from `max_retries + 1`) | +| `verdict` | `string` | `"approved"` or `"rejected"` | +| `feedback` | `string` | Reviewer feedback (empty string if approved) | +| `skill` | `string` | Name of the skill that performed the review | +| `elapsed_seconds` | `float` | Time taken for this review cycle | +| `timestamp` | `string` | ISO 8601 UTC timestamp of cycle completion | + +## Validation Summary + +| Condition | Behavior | +|-----------|----------| +| File missing | No reviewer spawned | +| File empty | Reviewer spawns with no instructions, default retries | +| No frontmatter | Entire content used as instructions, default retries | +| Empty frontmatter | Prose used as instructions, default retries | +| Malformed YAML | Warning logged, prose used, default retries | +| Invalid `max_retries` type | Warning logged, default used | +| Unknown frontmatter fields | Silently ignored | + +## See Also + +- [Authoring Skills](../skills/authoring.md) — how to create and customize skills +- [Configuration](config.md) — environment variables including `AUTO_REVIEW_MAX_RETRIES` diff --git a/docs/skills/authoring.md b/docs/skills/authoring.md index bf08bd6c..6dcb86e7 100644 --- a/docs/skills/authoring.md +++ b/docs/skills/authoring.md @@ -102,6 +102,22 @@ Write `.forge/fix-plan.md` with: 3. Proposed fix approach ``` +## Auto-Review + +Skills can include a `review.md` file alongside `SKILL.md` to enable automatic quality gates. When present, Forge spawns a reviewer agent after the skill completes, evaluating the output against criteria you define. + +``` +skills/ +└── myteam/ + └── implement-task/ + ├── SKILL.md + └── review.md ← reviewer instructions +``` + +The reviewer issues APPROVED or REJECTED verdicts. On rejection, the skill re-runs with feedback injected — up to `max_retries` attempts (default: 3). + +See the [Auto-Review Guide](../guide/auto-review.md) for configuration options and writing effective review instructions. + ## Using your skills with Forge See [Customize Forge for your project](../dev/contributing.md#customize-forge-for-your-project) for how to point a Jira project at your skills repo using `forge project-setup` and the skill installer. diff --git a/docs/skills/defaults.md b/docs/skills/defaults.md index b2a9f83f..edeb380b 100644 --- a/docs/skills/defaults.md +++ b/docs/skills/defaults.md @@ -50,6 +50,8 @@ Drives the code implementation agent running inside an ephemeral container. **Scope:** Given a task file at `.forge/task.json`, implement the code, run tests, and commit. No external network access. +**Auto-review:** Includes `review.md` that checks test coverage, error handling, documentation, and debug code before PR creation. See [Auto-Review Guide](../guide/auto-review.md). + --- ### `local-code-review` @@ -58,6 +60,8 @@ Reviews the implementation diff against `main` before PR creation. **Focus:** Breaking changes, test failures, security issues, and spec mismatches. Up to 2 fix passes. +**Auto-review:** Includes `review.md` that validates test suite execution and checks for breaking changes, security issues, and spec alignment. See [Auto-Review Guide](../guide/auto-review.md). + --- ### `analyze-ci` diff --git a/docs/skills/index.md b/docs/skills/index.md index 875c8c06..b3f55b90 100644 --- a/docs/skills/index.md +++ b/docs/skills/index.md @@ -40,7 +40,8 @@ skills/ │ └── review-code/ └── {project}/ # Per-project overrides (Jira key, lowercase) └── {skill-name}/ - └── SKILL.md + ├── SKILL.md + └── review.md # Optional auto-review config ``` ## Available Skills @@ -59,6 +60,12 @@ skills/ | `review-code` | AI PR review against spec | | `analyze-bug` | Bug RCA generation | +### Optional Files + +| File | Purpose | +|------|---------| +| `review.md` | Auto-review configuration — when present, Forge spawns a reviewer agent after skill execution. See [Auto-Review Guide](../guide/auto-review.md). | + ## The Primary Way to Contribute Writing a skill set for your team's stack is the fastest way to make Forge work better for your project — and to share that knowledge with others using similar tooling. diff --git a/skills/default/implement-task/review.md b/skills/default/implement-task/review.md new file mode 100644 index 00000000..0edd3484 --- /dev/null +++ b/skills/default/implement-task/review.md @@ -0,0 +1,213 @@ +--- +max_retries: 2 +--- + +# Implementation Review — 5-Dimension Deep Review + +Review the code changes by running 5 independent review passes. Run `git diff origin/main...HEAD` to see all changes. Read `.forge/task.json` to understand the task requirements. + +For each pass, evaluate independently — do not let findings in one pass influence another. Report all findings together at the end. + +--- + +## Pass 1: Simplicity / DRY / Elegance + +Is the code clean? Could anything be simpler? + +- Duplicated logic that should be extracted into a shared function +- Overly verbose code that could be expressed more concisely +- Unnecessary indirection or wrapper layers +- Complex conditionals that could be simplified +- Copy-pasted blocks with minor variations + +**Flag format:** `[Pass 1: Simplicity] file:line — description — severity` + +--- + +## Pass 2: Bugs / Functional Correctness + +Are there edge cases missed? Race conditions? Error handling gaps? + +- Unhandled error paths or bare `except:` blocks +- Off-by-one errors, boundary conditions, empty input handling +- Race conditions in async code (missing awaits, unsynchronized state) +- Resource leaks (unclosed files, connections, sessions) +- Type mismatches or incorrect assumptions about input shapes +- Security issues (injection, unsanitized input, exposed secrets) + +**Flag format:** `[Pass 2: Correctness] file:line — description — severity` + +--- + +## Pass 3: Project Conventions / Abstractions + +Does the code follow existing patterns in the codebase? + +- Naming conventions: `X | None` not `Optional[X]`, `StrEnum` for string enums +- `contextlib.suppress()` instead of empty try/except +- Unused parameters prefixed with `_` +- Logging via `logging.getLogger(__name__)`, not `print()` +- pytest patterns: fixtures, `pytest.raises()`, `@pytest.mark.asyncio` +- Type hints on all public functions with PEP 604 unions +- Abstractions that don't match the project's existing architecture + +**Flag format:** `[Pass 3: Conventions] file:line — description — severity` + +--- + +## Pass 4: Over-Engineering (Ponytail Review) + +Hunt for unnecessary complexity. The diff's best outcome is getting shorter. + +Tags: +- `delete:` dead code, unused flexibility, speculative feature. Replacement: nothing. +- `stdlib:` hand-rolled thing the standard library ships. Name the function. +- `native:` dependency or code doing what the platform already does. Name the feature. +- `yagni:` abstraction with one implementation, config nobody sets, layer with one caller. +- `shrink:` same logic, fewer lines. Show the shorter form. + +Scope: over-engineering and complexity only. Correctness bugs and security are covered by Pass 2. + +**Flag format:** `[Pass 4: Ponytail] file:line — tag: description. replacement. — severity` + +End this pass with: `net: - lines possible.` or `Lean already.` + +--- + +## Pass 5: Task-Implementation Alignment + +Read `.forge/task.json` carefully. Compare every requirement against the actual implementation. + +### 5a. Requirements Coverage + +For each requirement or acceptance criterion in the task: +- Is it implemented? Point to the specific code that fulfills it. +- Is it implemented correctly, or does the code only partially address it? +- Are there requirements that were missed entirely? +- Are there implemented features that were NOT requested (scope creep)? + +### 5b. Test Thoroughness + +For every piece of functionality implemented: +- Does a corresponding test exist? +- Does the test actually exercise the behavior, or is it shallow (e.g., only checks that a function is callable, only checks the happy path, asserts on type but not value)? +- Are edge cases tested (empty input, error paths, boundary values)? +- Are assertions specific? Tests that assert `is not None` or `isinstance()` without checking actual values are shallow. +- Do tests verify behavior, not implementation details? (e.g., testing the output, not that an internal method was called) + +### 5c. Gaps Summary + +List explicitly: +1. Requirements from the task that have NO implementation +2. Requirements that are implemented but have NO tests +3. Tests that exist but are shallow (explain why for each) +4. Functionality implemented that was not in the task requirements + +**Flag format:** `[Pass 5: Alignment] description — severity` + +--- + +## Severity Classification + +Classify every finding as one of: + +| Severity | Meaning | +|----------|---------| +| **critical** | Crashes, data loss, security holes, missing core requirements | +| **high** | Bugs in non-critical paths, missing error handling, missing tests for public API, shallow tests that don't verify behavior, partial requirement coverage, significant convention violations | +| **medium** | Minor convention violations, over-engineering | +| **low** | Minor style issues, small simplification opportunities, non-blocking suggestions | + +## Verdict + +Determine your verdict based on the review cycle. Check `.forge/reviews/` for previous cycle files — if none exist, this is cycle 1. + +### Cycle 1 (first review — no previous cycle files exist) + +Be strict. The implementation has not been reviewed yet, so catch everything up front. + +- Any finding of severity **medium or higher** → `REJECTED` +- Only **low** severity findings or no findings → `APPROVED` + +### Cycle 2+ (retry after feedback — previous cycle files exist) + +The implementation has already been revised based on prior feedback. Apply normal blocking rules. + +- Any **critical** or **high** severity finding → `REJECTED` +- Only **medium** or **low** findings or no findings → `APPROVED` + +Output exactly one marker: + +``` +APPROVED +``` + +``` +REJECTED +``` + +## Feedback Format + +On rejection (cycle 1 example — medium findings block): + +``` +REJECTED + +Pass 1 — Simplicity: +- [Pass 1: Simplicity] src/forge/module.py:42 — duplicated validation logic, extract to helper — medium + +Pass 2 — Correctness: +- [Pass 2: Correctness] src/forge/handler.py:87 — bare except swallows TypeError, use specific exception — high + +Pass 3 — Conventions: +- [Pass 3: Conventions] src/forge/utils.py:15 — uses Optional[str] instead of str | None — low + +Pass 4 — Ponytail: +- [Pass 4: Ponytail] src/forge/adapter.py:L30-55 — yagni: AbstractAdapter with one implementation. Inline it. — medium +- net: -25 lines possible. + +Pass 5 — Alignment: +- [Pass 5: Alignment] Task requires input validation for empty strings — no implementation found — critical +- [Pass 5: Alignment] test_handler.py:test_process only checks return type, not return value — shallow test — medium + +Required changes: +1. Implement empty string validation per task requirement +2. Add value assertion to test_process: verify actual output matches expected +3. Handle TypeError specifically in handler.py:87 +4. Extract duplicated validation logic into shared helper +5. Inline AbstractAdapter — only one implementation exists +``` + +On rejection (cycle 2+ example — only high/critical block): + +``` +REJECTED + +Pass 2 — Correctness: +- [Pass 2: Correctness] src/forge/handler.py:92 — new catch block still uses generic Exception, should catch httpx.HTTPError — high + +Pass 5 — Alignment: +- [Pass 5: Alignment] test_handler.py:test_empty_input asserts no exception raised but does not verify output value — medium (non-blocking on retry) + +Required changes: +1. Narrow exception type in handler.py:92 to httpx.HTTPError +``` + +On approval with notes: + +``` +APPROVED + +Pass 4 — Ponytail: +- [Pass 4: Ponytail] src/forge/utils.py:L12-18 — shrink: manual loop builds list. Use list comprehension. — low +- net: -4 lines possible. + +All task requirements implemented and tested. No blocking issues. +``` + +## Important + +- Be specific: include file path and line number for every finding +- Be actionable: state exactly what needs to change +- Read the FULL task description before starting Pass 5 — do not skim +- For Pass 5, err on the side of flagging shallow tests. A test that passes but proves nothing is worse than no test. diff --git a/src/forge/api/routes/metrics.py b/src/forge/api/routes/metrics.py index 42c49656..9776b2be 100644 --- a/src/forge/api/routes/metrics.py +++ b/src/forge/api/routes/metrics.py @@ -122,6 +122,26 @@ ["service", "operation", "error_type"], ) +# Review cycle metrics +REVIEW_CYCLES = Counter( + "forge_review_cycles_total", + "Total review cycles detected", + ["skill", "step"], +) + +REVIEW_VERDICTS = Counter( + "forge_review_verdicts_total", + "Review verdicts by outcome", + ["skill", "step", "verdict"], # verdict: approved, rejected +) + +REVIEW_DURATION = Histogram( + "forge_review_duration_seconds", + "Review cycle duration", + ["skill", "step"], + buckets=[1, 5, 10, 30, 60, 120, 300, 600], # Same as AGENT_DURATION +) + @router.get("/metrics") async def metrics() -> Response: @@ -232,3 +252,35 @@ def record_external_api_error(service: str, operation: str, error_type: str) -> error_type: Type of error (timeout, rate_limit, auth, etc.). """ EXTERNAL_API_ERRORS.labels(service=service, operation=operation, error_type=error_type).inc() + + +def record_review_cycle(skill: str, step: str) -> None: + """Record a review cycle detected. + + Args: + skill: Skill name (e.g., implement-task, fix-ci). + step: Workflow step name. + """ + REVIEW_CYCLES.labels(skill=skill, step=step).inc() + + +def record_review_verdict(skill: str, step: str, verdict: str) -> None: + """Record a review verdict. + + Args: + skill: Skill name (e.g., implement-task, fix-ci). + step: Workflow step name. + verdict: Verdict outcome (approved, rejected). + """ + REVIEW_VERDICTS.labels(skill=skill, step=step, verdict=verdict).inc() + + +def observe_review_duration(skill: str, step: str, duration: float) -> None: + """Record review cycle duration. + + Args: + skill: Skill name (e.g., implement-task, fix-ci). + step: Workflow step name. + duration: Duration in seconds. + """ + REVIEW_DURATION.labels(skill=skill, step=step).observe(duration) diff --git a/src/forge/config.py b/src/forge/config.py index e1bc7db9..79c64f27 100644 --- a/src/forge/config.py +++ b/src/forge/config.py @@ -330,6 +330,21 @@ def ignored_ci_checks(self) -> list[str]: description="Container CPU limit", ) + # Auto Review Configuration + auto_review_poll_interval: float = Field( + default=5.0, + description="Polling interval in seconds for detecting review cycle files during container execution", + ) + auto_review_record_polled_files: Literal["log", "copy"] | None = Field( + default=None, + description=( + "Recording mode for polled review cycle files. " + "'log' logs cycle data at INFO level. " + "'copy' copies files to {recording_dir}/{step-name}/review_cycle_*.json. " + "None disables recording." + ), + ) + # Queue Consumer Configuration queue_max_concurrent_tasks: int = Field( default=20, diff --git a/src/forge/observability/__init__.py b/src/forge/observability/__init__.py index b6fedc0d..cbd15eb6 100644 --- a/src/forge/observability/__init__.py +++ b/src/forge/observability/__init__.py @@ -10,6 +10,8 @@ get_correlation_id, set_correlation_id, ) +from forge.observability.review_poller import ReviewCycleData, ReviewCyclePoller +from forge.observability.review_recorder import ReviewCycleRecorder __all__ = [ "configure_tracing", @@ -18,4 +20,7 @@ "CorrelationContext", "get_correlation_id", "set_correlation_id", + "ReviewCycleData", + "ReviewCyclePoller", + "ReviewCycleRecorder", ] diff --git a/src/forge/observability/review_poller.py b/src/forge/observability/review_poller.py new file mode 100644 index 00000000..2da3b6aa --- /dev/null +++ b/src/forge/observability/review_poller.py @@ -0,0 +1,271 @@ +"""Review cycle poller for observability during container execution. + +This module provides the ReviewCyclePoller class that implements async polling +for review cycle files written by container agents during task execution. + +The poller detects files at the step-specific path: + .forge/{step-name}/review_cycle_*.json + +where step-name (e.g., "implement_task", "generate_prd") is passed when +creating the poller instance. +""" + +import asyncio +import json +import logging +from dataclasses import dataclass +from pathlib import Path + +from forge.config import Settings, get_settings + +logger = logging.getLogger(__name__) + +# Maximum retries for JSON parsing (race condition with container writes) +MAX_JSON_PARSE_RETRIES = 3 +# Delay between JSON parse retries in seconds +JSON_PARSE_RETRY_DELAY = 0.5 + + +@dataclass +class ReviewCycleData: + """Data captured for a single review cycle iteration. + + This mirrors the ReviewCycleData from containers.review for use in + the orchestrator-side polling. + + Attributes: + cycle: Current cycle number (1-indexed). + max_cycles: Maximum cycles allowed. + verdict: Review outcome ("approved" or "rejected"). + feedback: Reviewer feedback text. + skill: Name of the skill that performed the review. + elapsed_seconds: Time taken for this review cycle. + timestamp: ISO 8601 UTC timestamp of cycle completion. + file_path: Path to the source JSON file (not serialized). + """ + + cycle: int + max_cycles: int + verdict: str + feedback: str + skill: str + elapsed_seconds: float + timestamp: str + file_path: str = "" + + @classmethod + def from_dict(cls, data: dict, file_path: str = "") -> "ReviewCycleData": + """Create ReviewCycleData from a dictionary. + + Args: + data: Dictionary containing review cycle fields. + file_path: Path to the source file for tracking. + + Returns: + ReviewCycleData instance. + """ + return cls( + cycle=data["cycle"], + max_cycles=data["max_cycles"], + verdict=data["verdict"], + feedback=data.get("feedback", ""), + skill=data.get("skill", ""), + elapsed_seconds=data.get("elapsed_seconds", 0.0), + timestamp=data.get("timestamp", ""), + file_path=file_path, + ) + + +class ReviewCyclePoller: + """Async poller for review cycle files during container execution. + + This class polls for review_cycle_*.json files in the step-specific + directory and returns newly detected ReviewCycleData objects. + + Usage: + poller = ReviewCyclePoller( + workspace_path=Path("/workspace"), + step_name="implement_task", + ) + + # Start polling in background + async for new_cycles in poller.poll(): + for cycle in new_cycles: + print(f"Review cycle {cycle.cycle}: {cycle.verdict}") + + # Or poll once + new_cycles = await poller.poll_once() + """ + + def __init__( + self, + workspace_path: Path, + step_name: str, + task_key: str = "", + skill_name: str = "", + settings: Settings | None = None, + ): + """Initialize the review cycle poller. + + Args: + workspace_path: Path to the workspace root (where .forge/ is located). + step_name: Name of the step (e.g., "implement_task") for metrics. + task_key: Jira task key (e.g., "AISOS-2126") for directory naming. + skill_name: Skill name (e.g., "implement-task") for directory naming. + settings: Application settings. Uses default if not provided. + """ + self.workspace_path = Path(workspace_path) + self.step_name = step_name + self.task_key = task_key + self.skill_name = skill_name + self._settings = settings + self._processed_files: set[str] = set() + self._running = False + + @property + def settings(self) -> Settings: + """Get settings, lazily loading if not provided.""" + if self._settings is None: + self._settings = get_settings() + return self._settings + + @property + def poll_interval(self) -> float: + """Get the polling interval from settings.""" + return self.settings.auto_review_poll_interval + + @staticmethod + def build_cycle_dir( + workspace_path: Path, task_key: str, skill_name: str, step_name: str + ) -> Path: + """Build the directory path for review cycle files.""" + if task_key and skill_name: + return workspace_path / ".forge" / "reviews" / f"{task_key}__{skill_name}" + return workspace_path / ".forge" / step_name + + @property + def review_cycle_dir(self) -> Path: + """Get the directory path for review cycle files.""" + return self.build_cycle_dir( + self.workspace_path, self.task_key, self.skill_name, self.step_name + ) + + def _get_review_cycle_files(self) -> list[Path]: + """Get list of review_cycle_*.json files in the step directory. + + Returns: + List of Path objects for matching files. + """ + cycle_dir = self.review_cycle_dir + if not cycle_dir.exists(): + return [] + + return sorted(cycle_dir.glob("review_cycle_*.json")) + + async def _parse_json_with_retry(self, file_path: Path) -> dict | None: + """Parse JSON file with retry for incomplete reads. + + This handles race conditions where the container may still be + writing the file when we try to read it. + + Args: + file_path: Path to the JSON file. + + Returns: + Parsed JSON dict, or None if parsing fails after retries. + """ + for attempt in range(MAX_JSON_PARSE_RETRIES): + try: + content = file_path.read_text(encoding="utf-8") + + if not content.strip(): + # Empty file, likely still being written + if attempt < MAX_JSON_PARSE_RETRIES - 1: + logger.debug( + "Empty file %s, retrying (%d/%d)", + file_path, + attempt + 1, + MAX_JSON_PARSE_RETRIES, + ) + await asyncio.sleep(JSON_PARSE_RETRY_DELAY) + continue + return None + + return json.loads(content) + + except json.JSONDecodeError as e: + if attempt < MAX_JSON_PARSE_RETRIES - 1: + logger.debug( + "JSON parse error for %s: %s, retrying (%d/%d)", + file_path, + e, + attempt + 1, + MAX_JSON_PARSE_RETRIES, + ) + await asyncio.sleep(JSON_PARSE_RETRY_DELAY) + else: + logger.warning( + "Failed to parse %s after %d attempts: %s", + file_path, + MAX_JSON_PARSE_RETRIES, + e, + ) + return None + + except OSError as e: + logger.warning("Error reading %s: %s", file_path, e) + return None + + return None + + async def poll_once(self) -> list[ReviewCycleData]: + """Poll for new review cycle files once. + + Returns: + List of newly detected ReviewCycleData objects. + """ + new_cycles: list[ReviewCycleData] = [] + files = self._get_review_cycle_files() + + for file_path in files: + file_key = str(file_path) + + if file_key in self._processed_files: + continue + + data = await self._parse_json_with_retry(file_path) + if data is None: + continue + + try: + cycle_data = ReviewCycleData.from_dict(data, file_path=file_key) + new_cycles.append(cycle_data) + self._processed_files.add(file_key) + logger.debug( + "Detected review cycle %d for step %s: %s", + cycle_data.cycle, + self.step_name, + cycle_data.verdict, + ) + except (KeyError, TypeError) as e: + logger.warning("Invalid review cycle data in %s: %s", file_path, e) + + return new_cycles + + def stop(self) -> None: + """Stop the polling loop.""" + self._running = False + + async def run_loop(self, callback) -> None: + """Poll for review cycle files, calling callback(cycles) on each batch. + + Runs until stop() is called or the task is cancelled. + """ + self._running = True + while self._running: + await asyncio.sleep(self.poll_interval) + if not self._running: + break + new_cycles = await self.poll_once() + if new_cycles: + callback(new_cycles) diff --git a/src/forge/observability/review_recorder.py b/src/forge/observability/review_recorder.py new file mode 100644 index 00000000..504165c6 --- /dev/null +++ b/src/forge/observability/review_recorder.py @@ -0,0 +1,160 @@ +"""Review cycle recorder for observability of review cycle data. + +This module provides the ReviewCycleRecorder class that records review cycle +data either by logging or by copying files to a recording directory. + +Modes: + - mode="log": Log cycle data at INFO level via logging + - mode="copy": Copy files to {recording_dir}/{step-name}/review_cycle_*.json + - mode=None: No recording (disabled) + +The step name is passed to the recorder constructor and used to organize +recorded files into step-specific subdirectories. +""" + +import logging +import shutil +from pathlib import Path +from typing import Literal + +from forge.observability.review_poller import ReviewCycleData + +logger = logging.getLogger(__name__) + +# Type alias for recording modes +RecordingMode = Literal["log", "copy"] | None + + +class ReviewCycleRecorder: + """Records review cycle data based on configured mode. + + This class handles recording of review cycle data either by logging + to stdout/file or by copying files to a recording directory. + + Usage: + recorder = ReviewCycleRecorder( + step_name="implement_task", + mode="log", + ) + recorder.record(cycle_data) + + # Or for copy mode + recorder = ReviewCycleRecorder( + step_name="implement_task", + mode="copy", + recording_dir=Path("/recordings"), + ) + recorder.record_file(Path("/workspace/.forge/step/review_cycle_1.json")) + """ + + def __init__( + self, + step_name: str, + mode: RecordingMode = None, + recording_dir: Path | None = None, + ): + """Initialize the review cycle recorder. + + Args: + step_name: Name of the step (e.g., "implement_task") for file organization. + mode: Recording mode - "log", "copy", or None (disabled). + recording_dir: Base directory for copying files (required for copy mode). + + Raises: + ValueError: If mode is "copy" but recording_dir is not provided. + """ + self.step_name = step_name + self.mode = mode + self.recording_dir = Path(recording_dir) if recording_dir else None + + if mode == "copy" and not self.recording_dir: + raise ValueError("recording_dir is required when mode='copy'") + + @property + def step_dir(self) -> Path | None: + """Get the step-specific directory for recorded files. + + Returns: + Path to {recording_dir}/{step-name}/ or None if no recording_dir. + """ + if self.recording_dir is None: + return None + return self.recording_dir / self.step_name + + def record(self, cycle_data: ReviewCycleData) -> None: + """Record review cycle data based on configured mode. + + For log mode, logs the data at INFO level with structured format. + For copy mode, this method does nothing (use record_file for files). + For disabled mode (None), this method does nothing. + + Args: + cycle_data: The review cycle data to record. + """ + if self.mode is None: + return + + if self.mode == "log": + self._log_cycle_data(cycle_data) + # Copy mode is handled by record_file + + def record_file(self, source_file: Path) -> Path | None: + """Record a review cycle file by copying it to the recording directory. + + Only works in copy mode. Creates the step-specific subdirectory if needed. + + Args: + source_file: Path to the source review_cycle_*.json file. + + Returns: + Path to the copied file, or None if mode is not "copy" or copy failed. + """ + if self.mode != "copy": + return None + + if not self.recording_dir: + return None + + if not source_file.exists(): + logger.warning("Source file does not exist: %s", source_file) + return None + + # Create step-specific subdirectory + step_dir = self.step_dir + if step_dir is None: + return None + + try: + step_dir.mkdir(parents=True, exist_ok=True) + except OSError as e: + logger.error("Failed to create directory %s: %s", step_dir, e) + return None + + # Copy file preserving the filename + dest_file = step_dir / source_file.name + try: + shutil.copy2(source_file, dest_file) + logger.debug("Copied review cycle file to %s", dest_file) + return dest_file + except OSError as e: + logger.error("Failed to copy file to %s: %s", dest_file, e) + return None + + def _log_cycle_data(self, cycle_data: ReviewCycleData) -> None: + """Log review cycle data at INFO level with structured format. + + Args: + cycle_data: The review cycle data to log. + """ + logger.info( + "Review cycle %d/%d for %s: verdict=%s skill=%s elapsed=%.2fs feedback=%r", + cycle_data.cycle, + cycle_data.max_cycles, + self.step_name, + cycle_data.verdict, + cycle_data.skill, + cycle_data.elapsed_seconds, + cycle_data.feedback[:100] + "..." + if len(cycle_data.feedback) > 100 + else cycle_data.feedback, + ) diff --git a/src/forge/sandbox/runner.py b/src/forge/sandbox/runner.py index a422ef91..2e900a37 100644 --- a/src/forge/sandbox/runner.py +++ b/src/forge/sandbox/runner.py @@ -13,6 +13,7 @@ """ import asyncio +import contextlib import json import logging import os @@ -21,12 +22,39 @@ from pathlib import Path from typing import Any +from forge.api.routes.metrics import ( + observe_review_duration, + record_review_cycle, + record_review_verdict, +) from forge.config import Settings, get_settings +from forge.observability import ( + ReviewCycleData, + ReviewCyclePoller, + ReviewCycleRecorder, +) from forge.prompts import load_prompt from forge.skills.resolver import resolve_skill_paths logger = logging.getLogger(__name__) + +def _process_cycle( + cycle: ReviewCycleData, + step_name: str, + recorder: "ReviewCycleRecorder", + collected_cycles: list[ReviewCycleData], +) -> None: + """Record a review cycle: append, log via recorder, emit Prometheus metrics.""" + collected_cycles.append(cycle) + recorder.record(cycle) + if cycle.file_path: + recorder.record_file(Path(cycle.file_path)) + record_review_cycle(cycle.skill, step_name) + record_review_verdict(cycle.skill, step_name, cycle.verdict) + observe_review_duration(cycle.skill, step_name, cycle.elapsed_seconds) + + # Default container image (can be overridden via CONTAINER_IMAGE env var) # Use localhost/ prefix to avoid podman short-name resolution prompts DEFAULT_IMAGE = "localhost/forge-dev:latest" @@ -48,12 +76,21 @@ class ContainerResult: stderr: str tests_passed: bool | None = None # None if tests were skipped error_message: str | None = None + review_cycles: list[ReviewCycleData] = field(default_factory=list) @property def tests_failed(self) -> bool: """Check if tests specifically failed.""" return self.exit_code == EXIT_TESTS_FAILED + @property + def review_exhausted(self) -> bool: + """Check if the review loop exhausted all retries without approval.""" + if not self.review_cycles: + return False + last = self.review_cycles[-1] + return last.verdict == "rejected" and last.cycle >= last.max_cycles + @dataclass class ContainerConfig: @@ -366,6 +403,287 @@ async def _stop_timed_out_container( process.kill() await process.wait() + def _sweep_review_cycles( + self, + workspace_path: Path, + step_name: str, + processed_files: set[str], + collected_cycles: list[ReviewCycleData], + recorder: ReviewCycleRecorder, + task_key: str = "", + skill_name: str = "", + ) -> None: + """Synchronous post-execution sweep for missed review cycle files. + + This method scans for any review_cycle_*.json files that may have been + missed during async polling, especially if the container exits quickly + after writing. + + Args: + workspace_path: Path to the workspace root. + step_name: Name of the step for metrics. + processed_files: Set of file paths already processed by the poller. + collected_cycles: List to append newly found cycles to. + recorder: Recorder for logging/copying detected cycles. + task_key: Jira task key for directory naming. + skill_name: Skill name for directory naming. + """ + cycle_dir = ReviewCyclePoller.build_cycle_dir( + workspace_path, task_key, skill_name, step_name + ) + if not cycle_dir.exists(): + return + + # Find all review cycle files + all_files = sorted(cycle_dir.glob("review_cycle_*.json")) + + missed_count = 0 + for file_path in all_files: + file_key = str(file_path) + + # Skip files already processed by the async poller + if file_key in processed_files: + continue + + # This file was missed during polling - parse and collect it + try: + content = file_path.read_text(encoding="utf-8") + if not content.strip(): + logger.warning("Empty review cycle file during sweep: %s", file_path) + continue + + data = json.loads(content) + cycle_data = ReviewCycleData.from_dict(data, file_path=file_key) + missed_count += 1 + _process_cycle(cycle_data, step_name, recorder, collected_cycles) + logger.debug( + "Sweep caught review cycle %d/%d for %s: %s", + cycle_data.cycle, + cycle_data.max_cycles, + step_name, + cycle_data.verdict, + ) + + except json.JSONDecodeError as e: + logger.warning("Failed to parse review cycle file %s: %s", file_path, e) + except (KeyError, TypeError) as e: + logger.warning("Invalid review cycle data in %s: %s", file_path, e) + except OSError as e: + logger.warning("Error reading review cycle file %s: %s", file_path, e) + + if missed_count > 0: + logger.warning( + "Sweep caught %d review cycle file(s) missed during async polling for step %s", + missed_count, + step_name, + ) + + async def _poll_review_cycles( + self, + poller: ReviewCyclePoller, + recorder: ReviewCycleRecorder, + collected_cycles: list[ReviewCycleData], + ) -> None: + """Background task to poll for review cycle files during container execution. + + This task polls the workspace for review_cycle_*.json files and: + - Collects detected ReviewCycleData into the provided list + - Records cycles via the recorder (log or copy mode) + - Emits Prometheus metrics for observability + + Args: + poller: The ReviewCyclePoller instance to use for polling. + recorder: The ReviewCycleRecorder for recording cycles. + collected_cycles: List to aggregate detected cycles into. + """ + + def on_cycles(new_cycles: list[ReviewCycleData]) -> None: + for cycle in new_cycles: + _process_cycle(cycle, poller.step_name, recorder, collected_cycles) + + try: + await poller.run_loop(on_cycles) + except asyncio.CancelledError: + logger.debug("Review polling task cancelled") + raise + + async def _start_review_polling( + self, + workspace_path: Path, + step_name: str | None, + task_key: str, + skill_name: str, + collected_cycles: list[ReviewCycleData], + ) -> tuple[ReviewCyclePoller | None, ReviewCycleRecorder | None, asyncio.Task | None]: + """Create review poller, recorder, and start background polling task. + + Args: + workspace_path: Path to the workspace root. + step_name: Workflow step name for organizing review files. + If not provided, polling is disabled and (None, None, None) is returned. + task_key: Jira task key for directory naming. + skill_name: Skill name for directory naming. + collected_cycles: List to aggregate detected cycles into. + + Returns: + Tuple of (poller, recorder, polling_task), or (None, None, None) + if step_name is not provided. + """ + if not step_name: + return None, None, None + + poller = ReviewCyclePoller( + workspace_path=workspace_path, + step_name=step_name, + task_key=task_key, + skill_name=skill_name, + settings=self.settings, + ) + record_mode = self.settings.auto_review_record_polled_files + if record_mode == "copy": + logger.warning( + "Review recording mode 'copy' is not yet supported " + "(no recording_dir configured), falling back to 'log'" + ) + record_mode = "log" + recorder = ReviewCycleRecorder( + step_name=step_name, + mode=record_mode, + recording_dir=None, + ) + polling_task = asyncio.create_task( + self._poll_review_cycles(poller, recorder, collected_cycles) + ) + logger.debug(f"Started review polling for step: {step_name}") + return poller, recorder, polling_task + + async def _finalize_review_polling( + self, + poller: ReviewCyclePoller | None, + recorder: ReviewCycleRecorder | None, + polling_task: asyncio.Task | None, + workspace_path: Path, + step_name: str | None, + task_key: str, + skill_name: str, + collected_cycles: list[ReviewCycleData], + ) -> None: + """Stop review poller, cancel polling task, and sweep for missed files. + + Args: + poller: The ReviewCyclePoller instance, or None if polling was disabled. + recorder: The ReviewCycleRecorder instance, or None if polling was disabled. + polling_task: The background polling asyncio.Task, or None if polling was disabled. + workspace_path: Path to the workspace root. + step_name: Workflow step name for organizing review files. + task_key: Jira task key for directory naming. + skill_name: Skill name for directory naming. + collected_cycles: List to aggregate detected cycles into. + """ + if not polling_task or not poller or not recorder or not step_name: + return + + # Stop the poller + poller.stop() + # Cancel the polling task + polling_task.cancel() + with contextlib.suppress(asyncio.CancelledError): + await polling_task + logger.debug("Review polling task stopped") + + # Do one final async poll to catch any remaining files + final_cycles = await poller.poll_once() + for cycle in final_cycles: + _process_cycle(cycle, poller.step_name, recorder, collected_cycles) + + # Synchronous sweep for any files missed during async polling + # This catches files written just before container exit that may + # not have been detected by the async poller + self._sweep_review_cycles( + workspace_path=workspace_path, + step_name=step_name, + processed_files=poller._processed_files, + collected_cycles=collected_cycles, + recorder=recorder, + task_key=task_key, + skill_name=skill_name, + ) + + def _build_container_result( + self, + exit_code: int, + stdout_str: str, + stderr_str: str, + collected_cycles: list[ReviewCycleData], + container_name: str, + ) -> ContainerResult: + """Map container exit code to a ContainerResult. + + Handles logging of container output at appropriate levels and + emits the container_keep debugging warning when applicable. + + Args: + exit_code: Process exit code. + stdout_str: Decoded container stdout. + stderr_str: Decoded container stderr. + collected_cycles: Review cycles collected during execution. + container_name: Container name for log messages. + + Returns: + ContainerResult reflecting the exit status. + """ + logger.info(f"Container exited with code {exit_code}") + + # Log container output + if exit_code != EXIT_SUCCESS: + # Failure: stderr at INFO, stdout at DEBUG + if stderr_str: + logger.info(f"Container stderr:\n{stderr_str}") + if stdout_str: + logger.debug(f"Container stdout:\n{stdout_str}") + if self.settings.container_keep: + logger.warning( + f"Container kept for debugging (FORGE_CONTAINER_KEEP=true): " + f"{container_name}\n" + f" Inspect logs: podman logs {container_name}\n" + f" Enter filesystem: podman export {container_name} | tar -xC /tmp/{container_name}\n" + f" Remove when done: podman rm {container_name}" + ) + else: + # Success: stderr at DEBUG only + if stderr_str: + logger.debug(f"Container stderr:\n{stderr_str}") + + # Determine result + if exit_code == EXIT_SUCCESS: + return ContainerResult( + success=True, + exit_code=exit_code, + stdout=stdout_str, + stderr=stderr_str, + tests_passed=True, + review_cycles=collected_cycles, + ) + elif exit_code == EXIT_TESTS_FAILED: + return ContainerResult( + success=False, + exit_code=exit_code, + stdout=stdout_str, + stderr=stderr_str, + tests_passed=False, + error_message="Tests failed after max retries", + review_cycles=collected_cycles, + ) + else: + return ContainerResult( + success=False, + exit_code=exit_code, + stdout=stdout_str, + stderr=stderr_str, + error_message=f"Task failed with exit code {exit_code}", + review_cycles=collected_cycles, + ) + async def run( self, workspace_path: Path, @@ -377,6 +695,8 @@ async def run( repo_name: str | None = None, previous_task_keys: list[str] | None = None, trace_context: dict[str, Any] | None = None, + step_name: str | None = None, + skill_name: str | None = None, ) -> ContainerResult: """Run a task in a container sandbox. @@ -390,9 +710,12 @@ async def run( repo_name: Repository name (e.g., "owner/repo") for container naming. previous_task_keys: List of previously implemented task keys for handoff context. trace_context: Workflow fields forwarded to Langfuse only. + step_name: Workflow step name (e.g., "implement_task", "local_review") + for organizing review cycle files under .forge/{step-name}/. + If not provided, review polling is disabled. Returns: - ContainerResult with execution status and logs. + ContainerResult with execution status, logs, and review_cycles. """ config = config or self._default_config() @@ -406,9 +729,16 @@ async def run( "description": task_description, "previous_task_keys": previous_task_keys or [], "trace_context": trace_context or {}, + "skill_name": skill_name or "", } task_file.write_text(json.dumps(task_data, indent=2)) + # List to collect review cycles detected during execution + collected_cycles: list[ReviewCycleData] = [] + poller: ReviewCyclePoller | None = None + recorder: ReviewCycleRecorder | None = None + polling_task: asyncio.Task | None = None + try: # Build container name and command container_name = self._build_container_name(ticket_key, repo_name) @@ -426,6 +756,15 @@ async def run( stderr=asyncio.subprocess.PIPE, ) + # Start review polling background task if step_name is provided + poller, recorder, polling_task = await self._start_review_polling( + workspace_path, + step_name, + task_key or "", + skill_name or "", + collected_cycles, + ) + try: stdout, stderr = await asyncio.wait_for( process.communicate(), @@ -440,64 +779,31 @@ async def run( stdout="", stderr="Container execution timed out", error_message="Timeout exceeded", + review_cycles=collected_cycles, ) except asyncio.CancelledError: logger.warning(f"Container execution cancelled, stopping {container_name}") await self._stop_timed_out_container(container_name, process) raise # Re-raise CancelledError + finally: + await self._finalize_review_polling( + poller, + recorder, + polling_task, + workspace_path, + step_name, + task_key or "", + skill_name or "", + collected_cycles, + ) exit_code = process.returncode or 0 stdout_str = stdout.decode("utf-8", errors="replace") stderr_str = stderr.decode("utf-8", errors="replace") - logger.info(f"Container exited with code {exit_code}") - - # Log container output - if exit_code != EXIT_SUCCESS: - # Failure: stderr at INFO, stdout at DEBUG - if stderr_str: - logger.info(f"Container stderr:\n{stderr_str}") - if stdout_str: - logger.debug(f"Container stdout:\n{stdout_str}") - if self.settings.container_keep: - logger.warning( - f"Container kept for debugging (FORGE_CONTAINER_KEEP=true): " - f"{container_name}\n" - f" Inspect logs: podman logs {container_name}\n" - f" Enter filesystem: podman export {container_name} | tar -xC /tmp/{container_name}\n" - f" Remove when done: podman rm {container_name}" - ) - else: - # Success: stderr at DEBUG only - if stderr_str: - logger.debug(f"Container stderr:\n{stderr_str}") - - # Determine result - if exit_code == EXIT_SUCCESS: - return ContainerResult( - success=True, - exit_code=exit_code, - stdout=stdout_str, - stderr=stderr_str, - tests_passed=True, - ) - elif exit_code == EXIT_TESTS_FAILED: - return ContainerResult( - success=False, - exit_code=exit_code, - stdout=stdout_str, - stderr=stderr_str, - tests_passed=False, - error_message="Tests failed after max retries", - ) - else: - return ContainerResult( - success=False, - exit_code=exit_code, - stdout=stdout_str, - stderr=stderr_str, - error_message=f"Task failed with exit code {exit_code}", - ) + return self._build_container_result( + exit_code, stdout_str, stderr_str, collected_cycles, container_name + ) finally: # Cleanup task file diff --git a/src/forge/workflow/base.py b/src/forge/workflow/base.py index 3b59fcc5..e0b21a9c 100644 --- a/src/forge/workflow/base.py +++ b/src/forge/workflow/base.py @@ -1,5 +1,6 @@ """Base workflow classes and state definitions.""" +import operator from abc import ABC, abstractmethod from datetime import datetime from typing import Annotated, Any, TypedDict @@ -63,6 +64,7 @@ class PRIntegrationState(TypedDict, total=False): persistence_retry_count: int review_push_pending: bool review_push_pending_updates: dict[str, Any] + review_exhaustion_report: Annotated[dict[str, Any], operator.or_] class CIIntegrationState(TypedDict, total=False): diff --git a/src/forge/workflow/bug/state.py b/src/forge/workflow/bug/state.py index 94718cc1..2239d73f 100644 --- a/src/forge/workflow/bug/state.py +++ b/src/forge/workflow/bug/state.py @@ -99,6 +99,7 @@ def create_initial_bug_state(ticket_key: str, **kwargs: Any) -> BugState: "repos_to_process": [], "repos_completed": [], "implemented_tasks": [], + "review_exhaustion_report": {}, "current_task_key": None, "ci_failed_checks": [], "ci_skipped_checks": [], diff --git a/src/forge/workflow/feature/state.py b/src/forge/workflow/feature/state.py index 69543d2f..d2fce355 100644 --- a/src/forge/workflow/feature/state.py +++ b/src/forge/workflow/feature/state.py @@ -102,6 +102,7 @@ def create_initial_feature_state(ticket_key: str, **kwargs: Any) -> FeatureState "repos_to_process": [], "repos_completed": [], "implemented_tasks": [], + "review_exhaustion_report": {}, "current_task_key": None, "parallel_execution_enabled": True, "parallel_branch_id": None, diff --git a/src/forge/workflow/nodes/ci_evaluator.py b/src/forge/workflow/nodes/ci_evaluator.py index 84eb5812..4cf88a36 100644 --- a/src/forge/workflow/nodes/ci_evaluator.py +++ b/src/forge/workflow/nodes/ci_evaluator.py @@ -17,7 +17,7 @@ from forge.workflow.nodes.code_review import run_post_change_review, sync_pr_description from forge.workflow.nodes.error_handler import notify_error from forge.workflow.nodes.workspace_setup import prepare_workspace -from forge.workflow.utils import update_state_timestamp +from forge.workflow.utils import merge_review_exhaustion, update_state_timestamp from forge.workflow.utils.jira_status import ( post_status_comment, remove_implementing_label, @@ -315,14 +315,17 @@ async def attempt_ci_fix(state: WorkflowState) -> WorkflowState: ) runner = ContainerRunner(settings) - await runner.run( + result = await runner.run( workspace_path=Path(workspace_path), task_summary=f"Analyze CI failures (attempt {attempt})", task_description=analysis_prompt, ticket_key=ticket_key, task_key=f"{ticket_key}-ci-analyze", repo_name=state.get("current_repo", ""), + step_name="analyze_ci", + skill_name="analyze-ci", ) + state = merge_review_exhaustion(state, result, ticket_key, "analyze_ci") if not fix_plan_file.exists(): logger.warning(f"No fix plan written for {ticket_key} — skipping fix phase") @@ -343,14 +346,17 @@ async def attempt_ci_fix(state: WorkflowState) -> WorkflowState: fix_prompt = load_prompt("fix-ci", fix_plan=fix_plan) runner = ContainerRunner(settings) - await runner.run( + result = await runner.run( workspace_path=Path(workspace_path), task_summary=f"Apply CI fix plan (attempt {attempt})", task_description=fix_prompt, ticket_key=ticket_key, task_key=f"{ticket_key}-ci-fix", repo_name=state.get("current_repo", ""), + step_name="fix_ci", + skill_name="fix-ci", ) + state = merge_review_exhaustion(state, result, ticket_key, "fix_ci") workspace = Workspace( path=Path(workspace_path), diff --git a/src/forge/workflow/nodes/code_review.py b/src/forge/workflow/nodes/code_review.py index 9223e844..0c1f50a6 100644 --- a/src/forge/workflow/nodes/code_review.py +++ b/src/forge/workflow/nodes/code_review.py @@ -60,15 +60,23 @@ async def run_post_change_review( ) runner = ContainerRunner(settings) - await runner.run( + result = await runner.run( workspace_path=Path(workspace_path), task_summary=f"Post-{label} code review", task_description=task_description, ticket_key=ticket_key, task_key=f"{ticket_key}-review-{label}", repo_name=current_repo, + step_name="code_review", + skill_name="review-code", ) + if result.review_exhausted: + logger.warning( + f"Post-{label} review exhausted retries for {ticket_key} " + "(exhaustion data not propagated to state from utility function)" + ) + git = GitOperations( Workspace( path=Path(workspace_path), diff --git a/src/forge/workflow/nodes/docs_updater.py b/src/forge/workflow/nodes/docs_updater.py index b454c3f2..b73ee8c0 100644 --- a/src/forge/workflow/nodes/docs_updater.py +++ b/src/forge/workflow/nodes/docs_updater.py @@ -7,7 +7,7 @@ from forge.prompts import load_prompt from forge.sandbox import ContainerRunner from forge.workflow.feature.state import FeatureState as WorkflowState -from forge.workflow.utils import update_state_timestamp +from forge.workflow.utils import merge_review_exhaustion, update_state_timestamp from forge.workspace.git_ops import GitOperations from forge.workspace.manager import Workspace @@ -59,8 +59,12 @@ async def update_documentation(state: WorkflowState) -> WorkflowState: ticket_key=ticket_key, task_key=f"{ticket_key}-docs", repo_name=current_repo, + step_name="update_docs", + skill_name="update-docs", ) + state = merge_review_exhaustion(state, result, ticket_key, "update_docs") + git = GitOperations( Workspace( path=Path(workspace_path), diff --git a/src/forge/workflow/nodes/implement_review.py b/src/forge/workflow/nodes/implement_review.py index d37a9eed..70086844 100644 --- a/src/forge/workflow/nodes/implement_review.py +++ b/src/forge/workflow/nodes/implement_review.py @@ -14,7 +14,7 @@ from forge.workflow.feature.state import FeatureState as WorkflowState from forge.workflow.nodes.code_review import run_post_change_review, sync_pr_description from forge.workflow.nodes.workspace_setup import prepare_workspace -from forge.workflow.utils import set_paused, update_state_timestamp +from forge.workflow.utils import merge_review_exhaustion, set_paused, update_state_timestamp from forge.workflow.utils.jira_status import post_status_comment logger = logging.getLogger(__name__) @@ -172,14 +172,17 @@ async def implement_review(state: WorkflowState) -> WorkflowState: analysis_prompt = load_prompt("implement-review", ticket_key=ticket_key) runner = ContainerRunner(settings) - await runner.run( + result = await runner.run( workspace_path=Path(workspace_path), task_summary=f"Analyze PR review feedback for {ticket_key}", task_description=analysis_prompt, ticket_key=ticket_key, task_key=f"{ticket_key}-review-analyze", repo_name=current_repo, + step_name="implement_review_analyze", + skill_name="implement-review", ) + state = merge_review_exhaustion(state, result, ticket_key, "implement_review_analyze") # ── Check for objections ────────────────────────────────────────────── objections_path = Path(workspace_path) / _REVIEW_OBJECTIONS_FILE @@ -214,14 +217,17 @@ async def implement_review(state: WorkflowState) -> WorkflowState: fix_prompt = load_prompt("implement-review-fix", ticket_key=ticket_key) runner = ContainerRunner(settings) - await runner.run( + result = await runner.run( workspace_path=Path(workspace_path), task_summary=f"Implement PR review plan for {ticket_key}", task_description=fix_prompt, ticket_key=ticket_key, task_key=f"{ticket_key}-review-fix", repo_name=current_repo, + step_name="implement_review_fix", + skill_name="implement-review", ) + state = merge_review_exhaustion(state, result, ticket_key, "implement_review_fix") # Commit any uncommitted changes the container left if git.has_uncommitted_changes(): diff --git a/src/forge/workflow/nodes/implementation.py b/src/forge/workflow/nodes/implementation.py index 5514cc26..4eef2545 100644 --- a/src/forge/workflow/nodes/implementation.py +++ b/src/forge/workflow/nodes/implementation.py @@ -25,7 +25,7 @@ push_to_fork_with_retry, ) from forge.workflow.nodes.workspace_setup import prepare_workspace -from forge.workflow.utils import update_state_timestamp +from forge.workflow.utils import merge_review_exhaustion, update_state_timestamp from forge.workflow.utils.jira_status import post_status_comment from forge.workspace.git_ops import GitOperations @@ -202,6 +202,8 @@ async def implement_task(state: WorkflowState) -> WorkflowState: ticket_key=ticket_key, task_key=current_task, repo_name=current_repo, + step_name="implement_task", + skill_name="implement-task", previous_task_keys=implemented_tasks, trace_context=_build_implementation_trace_context( state, @@ -210,6 +212,9 @@ async def implement_task(state: WorkflowState) -> WorkflowState: ), ) + # Collect review exhaustion data (if auto-review ran and exhausted) + state = merge_review_exhaustion(state, result, current_task, "implement_task") + if result.success: logger.info(f"Container completed successfully for {current_task}") diff --git a/src/forge/workflow/nodes/local_reviewer.py b/src/forge/workflow/nodes/local_reviewer.py index 26c1d0d7..d2446680 100644 --- a/src/forge/workflow/nodes/local_reviewer.py +++ b/src/forge/workflow/nodes/local_reviewer.py @@ -21,7 +21,7 @@ run_review_container, ) from forge.workflow.nodes.workspace_setup import prepare_workspace -from forge.workflow.utils import update_state_timestamp +from forge.workflow.utils import merge_review_exhaustion, update_state_timestamp from forge.workflow.utils.jira_status import post_status_comment from forge.workspace.git_ops import GitOperations @@ -171,16 +171,20 @@ async def _run_bug_review(state: WorkflowState, git: GitOperations) -> WorkflowS try: runner = ContainerRunner(settings) - _, output = await run_review_container( + result, output = await run_review_container( runner, workspace_path=Path(workspace_path), task_summary="Qualitative bug review — root cause and test coverage", + step_name="local_review", + skill_name="local-review-bug", task_description=task_description, ticket_key=ticket_key, task_key=f"{ticket_key}-qualreview", repo_name=current_repo, ) + state = merge_review_exhaustion(state, result, ticket_key, "local_review") + if git.has_uncommitted_changes(): git.stage_all() git.commit(f"[{ticket_key}] fix: address review feedback") @@ -327,7 +331,7 @@ async def _run_feature_review(state: WorkflowState, git: GitOperations) -> Workf try: runner = ContainerRunner(settings) - _, output = await run_review_container( + result, output = await run_review_container( runner, workspace_path=Path(workspace_path), task_summary="Local code review — fix breaking issues", @@ -335,8 +339,12 @@ async def _run_feature_review(state: WorkflowState, git: GitOperations) -> Workf ticket_key=ticket_key, task_key=f"{ticket_key}-review", repo_name=current_repo, + step_name="local_review", + skill_name="local-code-review", ) + state = merge_review_exhaustion(state, result, ticket_key, "local_review") + if git.has_uncommitted_changes(): git.stage_all() git.commit(f"[{ticket_key}] fix: address breaking issues found in local review") diff --git a/src/forge/workflow/nodes/plan_bug_fix.py b/src/forge/workflow/nodes/plan_bug_fix.py index c1c7f33b..f430c5e5 100644 --- a/src/forge/workflow/nodes/plan_bug_fix.py +++ b/src/forge/workflow/nodes/plan_bug_fix.py @@ -15,7 +15,7 @@ from forge.prompts import load_prompt from forge.sandbox import ContainerRunner from forge.workflow.bug.state import BugState -from forge.workflow.utils import set_paused, update_state_timestamp +from forge.workflow.utils import merge_review_exhaustion, set_paused, update_state_timestamp from forge.workflow.utils.jira_status import post_status_comment logger = logging.getLogger(__name__) @@ -141,8 +141,12 @@ async def _run_plan_container( task_description=task_description, ticket_key=ticket_key, task_key=f"{ticket_key}-plan", + step_name="plan_bug_fix", + skill_name="plan-bug-fix", ) + state = merge_review_exhaustion(state, result, ticket_key, "plan_bug_fix") + if not result.success: raise RuntimeError( f"Container failed with exit_code={result.exit_code}: {result.stderr}" diff --git a/src/forge/workflow/nodes/pr_creation.py b/src/forge/workflow/nodes/pr_creation.py index 35bd6d28..769c3c81 100644 --- a/src/forge/workflow/nodes/pr_creation.py +++ b/src/forge/workflow/nodes/pr_creation.py @@ -303,6 +303,31 @@ async def create_pull_request(state: WorkflowState) -> WorkflowState: attempt=0, ) + # Append auto-review exhaustion section AFTER sync_pr_description + # (sync rewrites the body with an AI agent that would drop this section) + if pr_number is not None: + exhaustion_section = _format_review_exhaustion_section( + state.get("review_exhaustion_report", {}) + ) + if exhaustion_section: + try: + pr_data = await github.get_pull_request( + pr_target.owner, pr_target.repo, pr_number + ) + current_body = pr_data.get("body", "") or "" + if "## Auto-Review Notes" in current_body: + logger.debug("PR body already contains Auto-Review Notes — skipping append") + else: + await github.update_pull_request( + pr_target.owner, + pr_target.repo, + pr_number, + body=current_body + "\n\n" + exhaustion_section, + ) + logger.info("Appended auto-review exhaustion section to PR body") + except Exception as e: + logger.warning(f"Failed to append review exhaustion section: {e}") + return update_state_timestamp( { **state, @@ -341,6 +366,37 @@ def _get_pr_title(state: WorkflowState, ticket_summary: str = "") -> str: ) +def _format_review_exhaustion_section(report: dict[str, dict]) -> str: + """Format review exhaustion data as a markdown section for the PR body.""" + if not report: + return "" + + lines = [ + "## Auto-Review Notes", + "", + "The following review criteria could not be resolved after all retry attempts.", + "Human reviewers should pay particular attention to these areas.", + "", + ] + + for entry in report.values(): + step = entry.get("step_name", "unknown") + task = entry.get("task_key", "unknown") + skill = entry.get("skill", "unknown") + max_retries = entry.get("max_retries", "?") + feedback = entry.get("final_feedback", "") + + lines.append(f"### {step} — {task}") + lines.append(f"**Skill:** {skill} | **Retries:** {max_retries}/{max_retries} exhausted") + lines.append("") + if feedback: + for feedback_line in feedback.split("\n"): + lines.append(f"> {feedback_line}") + lines.append("") + + return "\n".join(lines) + + def _build_pr_body( state: WorkflowState, implemented_tasks: list[str], diff --git a/src/forge/workflow/nodes/rca_analysis.py b/src/forge/workflow/nodes/rca_analysis.py index 534728ef..1c27d79a 100644 --- a/src/forge/workflow/nodes/rca_analysis.py +++ b/src/forge/workflow/nodes/rca_analysis.py @@ -11,7 +11,7 @@ from forge.prompts import load_prompt from forge.sandbox import ContainerRunner from forge.workflow.bug.state import BugState -from forge.workflow.utils import update_state_timestamp +from forge.workflow.utils import merge_review_exhaustion, update_state_timestamp from forge.workflow.utils.jira_status import post_status_comment logger = logging.getLogger(__name__) @@ -104,8 +104,12 @@ async def analyze_bug(state: BugState) -> BugState: task_description=task_description, ticket_key=ticket_key, task_key=f"{ticket_key}-analysis", + step_name="analyze_bug", + skill_name="analyze-bug", ) + state = merge_review_exhaustion(state, result, ticket_key, "analyze_bug") + if not result.success: raise RuntimeError( f"Container failed with exit_code={result.exit_code}: {result.stderr}" @@ -250,8 +254,12 @@ async def reflect_rca(state: BugState) -> BugState: task_description=task_description, ticket_key=ticket_key, task_key=task_key, + step_name="reflect_rca", + skill_name="reflect-rca", ) + state = merge_review_exhaustion(state, result, ticket_key, "reflect_rca") + if not result.success: raise RuntimeError( f"Reflection container failed with exit_code={result.exit_code}: {result.stderr}" diff --git a/src/forge/workflow/nodes/rebase.py b/src/forge/workflow/nodes/rebase.py index 88237f4d..e6b1ef42 100644 --- a/src/forge/workflow/nodes/rebase.py +++ b/src/forge/workflow/nodes/rebase.py @@ -16,7 +16,7 @@ from forge.sandbox import ContainerRunner from forge.workflow.feature.state import FeatureState as WorkflowState from forge.workflow.nodes.workspace_setup import get_workspace_manager -from forge.workflow.utils import update_state_timestamp +from forge.workflow.utils import merge_review_exhaustion, update_state_timestamp from forge.workflow.utils.jira_status import post_status_comment from forge.workspace.git_ops import GitOperations @@ -157,8 +157,11 @@ async def rebase_pr(state: WorkflowState) -> WorkflowState: ticket_key=ticket_key, task_key=f"{ticket_key}-rebase", repo_name=current_repo, + step_name="rebase", ) + state = merge_review_exhaustion(state, result, ticket_key, "rebase") + if result.exit_code != 0: logger.error( f"Conflict resolution container failed for {ticket_key}: exit {result.exit_code}" diff --git a/src/forge/workflow/nodes/review_utils.py b/src/forge/workflow/nodes/review_utils.py index a3d64c6a..071a72f7 100644 --- a/src/forge/workflow/nodes/review_utils.py +++ b/src/forge/workflow/nodes/review_utils.py @@ -115,6 +115,8 @@ async def run_review_container( repo_name: str, config: ContainerConfig | None = None, previous_task_keys: list[str] | None = None, + step_name: str | None = None, + skill_name: str | None = None, ) -> tuple[ContainerResult, str]: """Execute a review container and return its result and combined output.""" # Never allow a failed retry to reuse an earlier attempt's verdict. The @@ -138,6 +140,10 @@ async def run_review_container( kwargs["config"] = config if previous_task_keys is not None: kwargs["previous_task_keys"] = previous_task_keys + if step_name is not None: + kwargs["step_name"] = step_name + if skill_name is not None: + kwargs["skill_name"] = skill_name result = await runner.run(**kwargs) output = collect_review_output( workspace_path, diff --git a/src/forge/workflow/nodes/task_takeover_execution.py b/src/forge/workflow/nodes/task_takeover_execution.py index 0a5b0747..9c8b8850 100644 --- a/src/forge/workflow/nodes/task_takeover_execution.py +++ b/src/forge/workflow/nodes/task_takeover_execution.py @@ -14,7 +14,7 @@ ) from forge.workflow.nodes.workspace_setup import prepare_workspace from forge.workflow.task_takeover.state import TaskTakeoverState -from forge.workflow.utils import update_state_timestamp +from forge.workflow.utils import merge_review_exhaustion, update_state_timestamp logger = logging.getLogger(__name__) @@ -123,9 +123,14 @@ async def execute_task_changes(state: TaskTakeoverState) -> TaskTakeoverState: ticket_key=ticket_key, task_key=current_task, repo_name=current_repo, + step_name="task_takeover_execution", + skill_name="implement-task", previous_task_keys=state.get("implemented_tasks", []), ) + # Collect review exhaustion data (if auto-review ran and exhausted) + state = merge_review_exhaustion(state, result, current_task, "task_takeover_execution") + # Initialize GitOperations on the host to stage and commit committed = False commit_message = ( diff --git a/src/forge/workflow/nodes/task_takeover_review.py b/src/forge/workflow/nodes/task_takeover_review.py index 68e65265..45867d58 100644 --- a/src/forge/workflow/nodes/task_takeover_review.py +++ b/src/forge/workflow/nodes/task_takeover_review.py @@ -15,7 +15,7 @@ ) from forge.workflow.nodes.workspace_setup import prepare_workspace from forge.workflow.task_takeover.state import TaskTakeoverState as WorkflowState -from forge.workflow.utils import update_state_timestamp +from forge.workflow.utils import merge_review_exhaustion, update_state_timestamp from forge.workspace.git_ops import GitOperations from forge.workspace.manager import Workspace @@ -99,7 +99,7 @@ async def run_qualitative_review(state: WorkflowState) -> WorkflowState: ) runner = ContainerRunner(settings) - _, response = await run_review_container( + result, response = await run_review_container( runner, workspace_path=Path(workspace_path), task_summary=f"Review task takeover changes for {current_task}", @@ -109,8 +109,12 @@ async def run_qualitative_review(state: WorkflowState) -> WorkflowState: task_key=f"{current_task}-review", repo_name=current_repo, previous_task_keys=state.get("implemented_tasks", []), + step_name="task_takeover_review", + skill_name="local-code-review", ) + state = merge_review_exhaustion(state, result, ticket_key, "task_takeover_review") + # Parse verdict and feedback verdict, feedback = _parse_qualitative_review(response) diff --git a/src/forge/workflow/task_takeover/state.py b/src/forge/workflow/task_takeover/state.py index c4e59990..eed2d289 100644 --- a/src/forge/workflow/task_takeover/state.py +++ b/src/forge/workflow/task_takeover/state.py @@ -79,6 +79,7 @@ def create_initial_task_takeover_state(ticket_key: str, **kwargs: Any) -> TaskTa "implementation_push_pending": False, "implementation_push_pending_task": None, "persistence_retry_count": 0, + "review_exhaustion_report": {}, } defaults.update(kwargs) return cast(TaskTakeoverState, defaults) diff --git a/src/forge/workflow/utils/__init__.py b/src/forge/workflow/utils/__init__.py index 80a10fb1..9fadeb79 100644 --- a/src/forge/workflow/utils/__init__.py +++ b/src/forge/workflow/utils/__init__.py @@ -15,6 +15,12 @@ transition_tasks_to_in_progress, ) from forge.workflow.utils.qa_summary import post_qa_summary_if_needed +from forge.workflow.utils.review_report import ( + collect_review_exhaustion as collect_review_exhaustion, +) +from forge.workflow.utils.review_report import ( + merge_review_exhaustion as merge_review_exhaustion, +) # Nodes whose resume mapping is identical across all workflow types. # Used by route_entry / route_by_ticket_type to avoid copy-pasting the same @@ -82,6 +88,8 @@ def set_error(state: dict[str, Any], error: str) -> dict[str, Any]: __all__ = [ "CommentType", "classify_comment", + "collect_review_exhaustion", + "merge_review_exhaustion", "post_qa_summary_if_needed", "post_status_comment", "remove_implementing_label", diff --git a/src/forge/workflow/utils/review_report.py b/src/forge/workflow/utils/review_report.py new file mode 100644 index 00000000..f045b59e --- /dev/null +++ b/src/forge/workflow/utils/review_report.py @@ -0,0 +1,60 @@ +"""Review exhaustion reporting utility.""" + +from __future__ import annotations + +from typing import TYPE_CHECKING, Any + +if TYPE_CHECKING: + from forge.sandbox.runner import ContainerResult + + +def collect_review_exhaustion( + container_result: ContainerResult, + task_key: str, + step_name: str, +) -> tuple[str, dict[str, Any]] | None: + """Build exhaustion report entry if review cycles exhausted. + + Args: + container_result: Result from container execution. + task_key: Jira task key (e.g., "AISOS-2053"). + step_name: Workflow step name (e.g., "implement_task"). + + Returns: + Tuple of (key, data) to merge into state['review_exhaustion_report'], + or None if review passed or no review ran. + """ + if not container_result.review_exhausted: + return None + + cycles = container_result.review_cycles + last_cycle = cycles[-1] + key = f"{task_key}__{step_name}" + data = { + "task_key": task_key, + "step_name": step_name, + "skill": last_cycle.skill, + "max_retries": last_cycle.max_cycles, + "final_feedback": last_cycle.feedback, + "cycles": [ + {"cycle": c.cycle, "verdict": c.verdict, "feedback": c.feedback} for c in cycles + ], + } + return key, data + + +def merge_review_exhaustion( + state: dict[str, Any], + container_result: ContainerResult, + task_key: str, + step_name: str, +) -> dict[str, Any]: + """Merge review exhaustion data into state if review cycles were exhausted. + + Returns the state unchanged if review passed or no review ran. + """ + exhaustion = collect_review_exhaustion(container_result, task_key, step_name) + if exhaustion: + key, data = exhaustion + return {**state, "review_exhaustion_report": {key: data}} + return state diff --git a/tests/unit/api/routes/test_metrics.py b/tests/unit/api/routes/test_metrics.py index 21a962f1..8a4e8b00 100644 --- a/tests/unit/api/routes/test_metrics.py +++ b/tests/unit/api/routes/test_metrics.py @@ -12,10 +12,7 @@ class TestMetricsEndpoint: @pytest.mark.asyncio async def test_metrics_returns_200(self): """Metrics endpoint returns 200.""" - async with AsyncClient( - transport=ASGITransport(app=app), - base_url="http://test" - ) as client: + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: response = await client.get("/metrics") assert response.status_code == 200 @@ -23,10 +20,7 @@ async def test_metrics_returns_200(self): @pytest.mark.asyncio async def test_metrics_returns_prometheus_format(self): """Metrics endpoint returns Prometheus format.""" - async with AsyncClient( - transport=ASGITransport(app=app), - base_url="http://test" - ) as client: + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: response = await client.get("/metrics") content_type = response.headers.get("content-type", "") @@ -35,10 +29,7 @@ async def test_metrics_returns_prometheus_format(self): @pytest.mark.asyncio async def test_metrics_includes_forge_metrics(self): """Metrics includes forge-related counters.""" - async with AsyncClient( - transport=ASGITransport(app=app), - base_url="http://test" - ) as client: + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: response = await client.get("/metrics") body = response.text @@ -48,10 +39,7 @@ async def test_metrics_includes_forge_metrics(self): @pytest.mark.asyncio async def test_metrics_includes_workflow_metrics(self): """Metrics includes workflow-related counters.""" - async with AsyncClient( - transport=ASGITransport(app=app), - base_url="http://test" - ) as client: + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: response = await client.get("/metrics") body = response.text @@ -88,3 +76,133 @@ def test_increment_webhook_counter(self): # Should not raise WEBHOOKS_RECEIVED.labels(source="github", event_type="check_run").inc() + + +class TestReviewCycleMetrics: + """Tests for review cycle metrics registration and recording.""" + + def test_review_cycles_counter_exists(self): + """Review cycles counter is registered.""" + from forge.api.routes.metrics import REVIEW_CYCLES + + assert REVIEW_CYCLES is not None + + def test_review_cycles_counter_has_labels(self): + """Review cycles counter has skill and step labels.""" + from forge.api.routes.metrics import REVIEW_CYCLES + + labeled = REVIEW_CYCLES.labels(skill="implement-task", step="implementation") + assert labeled is not None + + def test_review_verdicts_counter_exists(self): + """Review verdicts counter is registered.""" + from forge.api.routes.metrics import REVIEW_VERDICTS + + assert REVIEW_VERDICTS is not None + + def test_review_verdicts_counter_has_labels(self): + """Review verdicts counter has skill, step, and verdict labels.""" + from forge.api.routes.metrics import REVIEW_VERDICTS + + labeled = REVIEW_VERDICTS.labels( + skill="implement-task", step="implementation", verdict="approved" + ) + assert labeled is not None + + def test_review_duration_histogram_exists(self): + """Review duration histogram is registered.""" + from forge.api.routes.metrics import REVIEW_DURATION + + assert REVIEW_DURATION is not None + + def test_review_duration_histogram_has_labels(self): + """Review duration histogram has skill and step labels.""" + from forge.api.routes.metrics import REVIEW_DURATION + + labeled = REVIEW_DURATION.labels(skill="fix-ci", step="ci_fix") + assert labeled is not None + + def test_record_review_cycle_helper(self): + """record_review_cycle helper increments counter.""" + from forge.api.routes.metrics import REVIEW_CYCLES, record_review_cycle + + # Get initial value + initial = REVIEW_CYCLES.labels(skill="test-skill", step="test-step")._value.get() + + record_review_cycle(skill="test-skill", step="test-step") + + # Counter should be incremented + final = REVIEW_CYCLES.labels(skill="test-skill", step="test-step")._value.get() + assert final == initial + 1 + + def test_record_review_verdict_helper(self): + """record_review_verdict helper increments counter.""" + from forge.api.routes.metrics import REVIEW_VERDICTS, record_review_verdict + + # Get initial value + initial = REVIEW_VERDICTS.labels( + skill="test-skill", step="test-step", verdict="approved" + )._value.get() + + record_review_verdict(skill="test-skill", step="test-step", verdict="approved") + + # Counter should be incremented + final = REVIEW_VERDICTS.labels( + skill="test-skill", step="test-step", verdict="approved" + )._value.get() + assert final == initial + 1 + + def test_record_review_verdict_rejected(self): + """record_review_verdict helper handles rejected verdict.""" + from forge.api.routes.metrics import REVIEW_VERDICTS, record_review_verdict + + # Get initial value + initial = REVIEW_VERDICTS.labels( + skill="task-skill", step="task-step", verdict="rejected" + )._value.get() + + record_review_verdict(skill="task-skill", step="task-step", verdict="rejected") + + # Counter should be incremented + final = REVIEW_VERDICTS.labels( + skill="task-skill", step="task-step", verdict="rejected" + )._value.get() + assert final == initial + 1 + + def test_observe_review_duration_helper(self): + """observe_review_duration helper observes histogram.""" + from forge.api.routes.metrics import REVIEW_DURATION, observe_review_duration + + # Get initial sum + initial_sum = REVIEW_DURATION.labels( + skill="duration-skill", step="duration-step" + )._sum.get() + + observe_review_duration(skill="duration-skill", step="duration-step", duration=45.5) + + # Sum should increase by observed value + final_sum = REVIEW_DURATION.labels(skill="duration-skill", step="duration-step")._sum.get() + # Just check that something was observed (sum increased) + assert final_sum >= initial_sum + 45.5 + + @pytest.mark.asyncio + async def test_review_metrics_in_endpoint_output(self): + """Review metrics appear in /metrics endpoint output.""" + from forge.api.routes.metrics import ( + observe_review_duration, + record_review_cycle, + record_review_verdict, + ) + + # Record some metrics first + record_review_cycle(skill="endpoint-skill", step="endpoint-step") + record_review_verdict(skill="endpoint-skill", step="endpoint-step", verdict="approved") + observe_review_duration(skill="endpoint-skill", step="endpoint-step", duration=30.0) + + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + response = await client.get("/metrics") + + body = response.text + assert "forge_review_cycles_total" in body + assert "forge_review_verdicts_total" in body + assert "forge_review_duration_seconds" in body diff --git a/tests/unit/containers/__init__.py b/tests/unit/containers/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/unit/containers/conftest.py b/tests/unit/containers/conftest.py new file mode 100644 index 00000000..74931205 --- /dev/null +++ b/tests/unit/containers/conftest.py @@ -0,0 +1,8 @@ +"""Container tests conftest - adds containers/ to sys.path.""" + +import sys +from pathlib import Path + +_containers_dir = str(Path(__file__).parents[3] / "containers") +if _containers_dir not in sys.path: + sys.path.insert(0, _containers_dir) diff --git a/tests/unit/containers/test_entrypoint_review.py b/tests/unit/containers/test_entrypoint_review.py new file mode 100644 index 00000000..9c5d6ce3 --- /dev/null +++ b/tests/unit/containers/test_entrypoint_review.py @@ -0,0 +1,1039 @@ +"""Unit tests for review loop integration in entrypoint.py.""" + +import json +import sys +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +# Add containers to path +sys.path.insert(0, str(Path(__file__).parents[3] / "containers")) + + +# --------------------------------------------------------------------------- +# Test _create_llm_model +# --------------------------------------------------------------------------- + + +class TestCreateLlmModel: + def test_raises_without_credentials(self, monkeypatch): + """Test that missing credentials raises RuntimeError.""" + monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False) + monkeypatch.delenv("ANTHROPIC_VERTEX_PROJECT_ID", raising=False) + + from entrypoint import _create_llm_model + + with pytest.raises(RuntimeError, match="No API credentials"): + _create_llm_model() + + def test_raises_gemini_without_vertex(self, monkeypatch): + """Test that Gemini model without Vertex AI raises RuntimeError.""" + monkeypatch.setenv("ANTHROPIC_API_KEY", "test-key") + monkeypatch.delenv("ANTHROPIC_VERTEX_PROJECT_ID", raising=False) + monkeypatch.setenv("LLM_MODEL", "gemini-pro") + + from entrypoint import _create_llm_model + + with pytest.raises(RuntimeError, match="requires Vertex AI"): + _create_llm_model() + + def test_creates_anthropic_model_with_api_key(self, monkeypatch): + """Test model creation with direct Anthropic API key.""" + monkeypatch.setenv("ANTHROPIC_API_KEY", "test-key") + monkeypatch.delenv("ANTHROPIC_VERTEX_PROJECT_ID", raising=False) + monkeypatch.setenv("LLM_MODEL", "claude-sonnet-4-5@20250929") + + from entrypoint import _create_llm_model + + name, model = _create_llm_model(max_tokens_default=8192) + assert name == "claude-sonnet-4-5@20250929" + assert model is not None + + def test_respects_max_tokens_env_override(self, monkeypatch): + """Test that LLM_MAX_TOKENS env var overrides default.""" + monkeypatch.setenv("ANTHROPIC_API_KEY", "test-key") + monkeypatch.delenv("ANTHROPIC_VERTEX_PROJECT_ID", raising=False) + monkeypatch.setenv("LLM_MODEL", "claude-sonnet-4-5@20250929") + monkeypatch.setenv("LLM_MAX_TOKENS", "4096") + + from entrypoint import _create_llm_model + + name, model = _create_llm_model(max_tokens_default=16384) + assert model is not None + + def test_creates_vertex_claude_model(self, monkeypatch): + """Test model creation with Vertex AI for Claude.""" + monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False) + monkeypatch.setenv("ANTHROPIC_VERTEX_PROJECT_ID", "my-project") + monkeypatch.setenv("ANTHROPIC_VERTEX_REGION", "us-east5") + monkeypatch.setenv("LLM_MODEL", "claude-sonnet-4-5@20250929") + + from entrypoint import _create_llm_model + + name, model = _create_llm_model() + assert name == "claude-sonnet-4-5@20250929" + assert model is not None + + +# --------------------------------------------------------------------------- +# Test run_reviewer_agent +# --------------------------------------------------------------------------- + + +class TestRunReviewerAgent: + @pytest.mark.asyncio + async def test_returns_agent_output(self, tmp_path: Path): + """Test that reviewer agent returns its output text.""" + from entrypoint import run_reviewer_agent + + mock_agent = MagicMock() + mock_agent.ainvoke = AsyncMock(return_value={"messages": [MagicMock(content="APPROVED")]}) + mock_create = MagicMock(return_value=mock_agent) + + with ( + patch("entrypoint._create_llm_model", return_value=("test-model", MagicMock())), + patch("deepagents.create_deep_agent", mock_create), + patch("deepagents.backends.LocalShellBackend"), + ): + result = await run_reviewer_agent( + workspace=tmp_path, + review_instructions="Check for bugs", + task_key="TEST-123", + ) + + assert result == "APPROVED" + + @pytest.mark.asyncio + async def test_system_prompt_contains_instructions(self, tmp_path: Path): + """Test that review instructions are included in system prompt.""" + from entrypoint import run_reviewer_agent + + mock_agent = MagicMock() + mock_agent.ainvoke = AsyncMock( + return_value={"messages": [MagicMock(content="REJECTED: bad code")]} + ) + mock_create = MagicMock(return_value=mock_agent) + + with ( + patch("entrypoint._create_llm_model", return_value=("test-model", MagicMock())), + patch("deepagents.create_deep_agent", mock_create), + patch("deepagents.backends.LocalShellBackend"), + ): + await run_reviewer_agent( + workspace=tmp_path, + review_instructions="Check for security issues", + task_key="TEST-456", + ) + + call_kwargs = mock_create.call_args[1] + assert "Check for security issues" in call_kwargs["system_prompt"] + + @pytest.mark.asyncio + async def test_uses_8192_max_tokens_default(self, tmp_path: Path): + """Test that reviewer uses 8192 as default max tokens.""" + from entrypoint import run_reviewer_agent + + mock_agent = MagicMock() + mock_agent.ainvoke = AsyncMock(return_value={"messages": [MagicMock(content="APPROVED")]}) + + with ( + patch( + "entrypoint._create_llm_model", return_value=("test-model", MagicMock()) + ) as mock_create_model, + patch("deepagents.create_deep_agent", return_value=mock_agent), + patch("deepagents.backends.LocalShellBackend"), + ): + await run_reviewer_agent( + workspace=tmp_path, + review_instructions="Review", + task_key="TEST-789", + ) + + mock_create_model.assert_called_once_with(max_tokens_default=8192) + + +# --------------------------------------------------------------------------- +# Test run_worker_with_feedback +# --------------------------------------------------------------------------- + + +class TestRunWorkerWithFeedback: + @pytest.mark.asyncio + async def test_injects_feedback_section(self, tmp_path: Path): + """Test that feedback is injected into task description (SC-003).""" + with patch("entrypoint.run_agent_task") as mock_run_agent: + mock_run_agent.return_value = True + + from entrypoint import run_worker_with_feedback + + result = await run_worker_with_feedback( + workspace=tmp_path, + task_key="TEST-123", + task_summary="Test task", + task_description="Original description", + guardrails="Some guardrails", + feedback="Please fix the error handling", + previous_task_keys=["TEST-122"], + ) + + assert result is True + mock_run_agent.assert_called_once() + + # Check that feedback was injected + call_kwargs = mock_run_agent.call_args + task_description = call_kwargs[1]["task_description"] + assert "## Reviewer Feedback" in task_description + assert "Please fix the error handling" in task_description + assert "Original description" in task_description + + @pytest.mark.asyncio + async def test_loads_conversation_history(self, tmp_path: Path): + """Test that conversation history is loaded (SC-003).""" + import sys + + sys.path.insert(0, str(Path(__file__).parents[3] / "containers")) + + # Create history file + history_dir = tmp_path / ".forge" / "history" + history_dir.mkdir(parents=True) + history_file = history_dir / "TEST-123.json" + history_data = {"task_key": "TEST-123", "messages": [{"role": "user", "content": "Test"}]} + history_file.write_text(json.dumps(history_data)) + + with patch("entrypoint.run_agent_task") as mock_run_agent: + mock_run_agent.return_value = True + + from entrypoint import run_worker_with_feedback + + result = await run_worker_with_feedback( + workspace=tmp_path, + task_key="TEST-123", + task_summary="Test task", + task_description="Original description", + guardrails="", + feedback="Feedback text", + ) + + assert result is True + + +# --------------------------------------------------------------------------- +# Test run_review_loop +# --------------------------------------------------------------------------- + + +class TestRunReviewLoop: + @pytest.mark.asyncio + async def test_approved_verdict_exits_successfully(self, tmp_path: Path): + """Test that APPROVED verdict terminates loop successfully (SC-002).""" + import sys + + sys.path.insert(0, str(Path(__file__).parents[3] / "containers")) + + # Create review.md + review_md = tmp_path / "review.md" + review_md.write_text("---\nmax_retries: 3\n---\nReview instructions here") + + with patch("entrypoint.run_reviewer_agent") as mock_reviewer: + mock_reviewer.return_value = "The code looks great. APPROVED" + + from entrypoint import run_review_loop + + result = await run_review_loop( + workspace=tmp_path, + task_key="TEST-123", + task_summary="Test task", + task_description="Description", + guardrails="", + skill_name="test-skill", + review_md_path=review_md, + ) + + assert result is True + mock_reviewer.assert_called_once() + + # Check cycle file was written + cycle_file = ( + tmp_path / ".forge" / "reviews" / "TEST-123__test-skill" / "review_cycle_1.json" + ) + assert cycle_file.exists() + cycle_data = json.loads(cycle_file.read_text()) + assert cycle_data["verdict"] == "approved" + assert cycle_data["cycle"] == 1 + + @pytest.mark.asyncio + async def test_rejected_triggers_retry(self, tmp_path: Path): + """Test that REJECTED verdict triggers retry with feedback (SC-003).""" + import sys + + sys.path.insert(0, str(Path(__file__).parents[3] / "containers")) + + # Create review.md with 2 retries + review_md = tmp_path / "review.md" + review_md.write_text("---\nmax_retries: 2\n---\nReview instructions") + + call_count = 0 + + async def mock_reviewer_side_effect(*_args, **_kwargs): + nonlocal call_count + call_count += 1 + if call_count == 1: + return "REJECTED: Missing error handling" + return "APPROVED" + + with ( + patch("entrypoint.run_reviewer_agent") as mock_reviewer, + patch("entrypoint.run_worker_with_feedback") as mock_worker, + ): + mock_reviewer.side_effect = mock_reviewer_side_effect + mock_worker.return_value = True + + from entrypoint import run_review_loop + + result = await run_review_loop( + workspace=tmp_path, + task_key="TEST-123", + task_summary="Test task", + task_description="Description", + guardrails="", + skill_name="test-skill", + review_md_path=review_md, + ) + + assert result is True + assert mock_reviewer.call_count == 2 + mock_worker.assert_called_once() + + # Check feedback was passed to worker + worker_call = mock_worker.call_args + assert "Missing error handling" in worker_call[1]["feedback"] + + @pytest.mark.asyncio + async def test_max_retries_exhausted_exits_success(self, tmp_path: Path): + """Test that max retries exhausted exits with success (BR-005).""" + import sys + + sys.path.insert(0, str(Path(__file__).parents[3] / "containers")) + + # Create review.md with 2 retries + review_md = tmp_path / "review.md" + review_md.write_text("---\nmax_retries: 2\n---\nReview instructions") + + with ( + patch("entrypoint.run_reviewer_agent") as mock_reviewer, + patch("entrypoint.run_worker_with_feedback") as mock_worker, + ): + # Always reject + mock_reviewer.return_value = "REJECTED: Still not good" + mock_worker.return_value = True + + from entrypoint import run_review_loop + + result = await run_review_loop( + workspace=tmp_path, + task_key="TEST-123", + task_summary="Test task", + task_description="Description", + guardrails="", + skill_name="test-skill", + review_md_path=review_md, + ) + + # Should exit with success even after max retries + assert result is True + assert mock_reviewer.call_count == 2 + + @pytest.mark.asyncio + async def test_uses_frontmatter_max_retries(self, tmp_path: Path): + """Test that max_retries from frontmatter is enforced (SC-004).""" + import sys + + sys.path.insert(0, str(Path(__file__).parents[3] / "containers")) + + # Create review.md with 5 retries + review_md = tmp_path / "review.md" + review_md.write_text("---\nmax_retries: 5\n---\nReview instructions") + + with ( + patch("entrypoint.run_reviewer_agent") as mock_reviewer, + patch("entrypoint.run_worker_with_feedback") as mock_worker, + ): + mock_reviewer.return_value = "REJECTED" + mock_worker.return_value = True + + from entrypoint import run_review_loop + + result = await run_review_loop( + workspace=tmp_path, + task_key="TEST-123", + task_summary="Test task", + task_description="Description", + guardrails="", + skill_name="test-skill", + review_md_path=review_md, + ) + + # Should have run 5 review cycles + assert mock_reviewer.call_count == 5 + assert result is True + + @pytest.mark.asyncio + async def test_uses_env_var_fallback(self, tmp_path: Path, monkeypatch): + """Test that AUTO_REVIEW_MAX_RETRIES env var is used as fallback (SC-005).""" + import sys + + sys.path.insert(0, str(Path(__file__).parents[3] / "containers")) + + monkeypatch.setenv("AUTO_REVIEW_MAX_RETRIES", "4") + + # Create review.md without frontmatter + review_md = tmp_path / "review.md" + review_md.write_text("Review instructions only, no frontmatter") + + with ( + patch("entrypoint.run_reviewer_agent") as mock_reviewer, + patch("entrypoint.run_worker_with_feedback") as mock_worker, + ): + mock_reviewer.return_value = "REJECTED" + mock_worker.return_value = True + + from entrypoint import run_review_loop + + result = await run_review_loop( + workspace=tmp_path, + task_key="TEST-123", + task_summary="Test task", + task_description="Description", + guardrails="", + skill_name="test-skill", + review_md_path=review_md, + ) + + # Should have run 4 review cycles (from env var) + assert mock_reviewer.call_count == 4 + assert result is True + + @pytest.mark.asyncio + async def test_cycle_file_written_to_correct_path(self, tmp_path: Path): + """Test that cycle file is written to .forge/{step_name}/review_cycle_N.json (SC-007).""" + import sys + + sys.path.insert(0, str(Path(__file__).parents[3] / "containers")) + + review_md = tmp_path / "review.md" + review_md.write_text("---\nmax_retries: 1\n---\nReview") + + with patch("entrypoint.run_reviewer_agent") as mock_reviewer: + mock_reviewer.return_value = "APPROVED" + + from entrypoint import run_review_loop + + await run_review_loop( + workspace=tmp_path, + task_key="TEST-123", + task_summary="Test", + task_description="Desc", + guardrails="", + skill_name="my-custom-skill", + review_md_path=review_md, + ) + + # Check file path matches spec + cycle_file = ( + tmp_path + / ".forge" + / "reviews" + / "TEST-123__my-custom-skill" + / "review_cycle_1.json" + ) + assert cycle_file.exists() + + @pytest.mark.asyncio + async def test_reviewer_agent_receives_instructions(self, tmp_path: Path): + """Test that reviewer agent receives review.md instructions (SC-001).""" + import sys + + sys.path.insert(0, str(Path(__file__).parents[3] / "containers")) + + review_md = tmp_path / "review.md" + review_md.write_text("---\nmax_retries: 1\n---\nCheck for security issues and code quality") + + with patch("entrypoint.run_reviewer_agent") as mock_reviewer: + mock_reviewer.return_value = "APPROVED" + + from entrypoint import run_review_loop + + await run_review_loop( + workspace=tmp_path, + task_key="TEST-123", + task_summary="Test", + task_description="Desc", + guardrails="", + skill_name="test-skill", + review_md_path=review_md, + ) + + # Check reviewer was called with instructions + call_kwargs = mock_reviewer.call_args[1] + assert "Check for security issues" in call_kwargs["review_instructions"] + + @pytest.mark.asyncio + async def test_cycle_timing_tracked(self, tmp_path: Path): + """Test that cycle timing is tracked with time.perf_counter().""" + import sys + + sys.path.insert(0, str(Path(__file__).parents[3] / "containers")) + + review_md = tmp_path / "review.md" + review_md.write_text("---\nmax_retries: 1\n---\nReview") + + with patch("entrypoint.run_reviewer_agent") as mock_reviewer: + mock_reviewer.return_value = "APPROVED" + + from entrypoint import run_review_loop + + await run_review_loop( + workspace=tmp_path, + task_key="TEST-123", + task_summary="Test", + task_description="Desc", + guardrails="", + skill_name="test-skill", + review_md_path=review_md, + ) + + cycle_file = ( + tmp_path / ".forge" / "reviews" / "TEST-123__test-skill" / "review_cycle_1.json" + ) + cycle_data = json.loads(cycle_file.read_text()) + + # elapsed_seconds should be a positive float + assert isinstance(cycle_data["elapsed_seconds"], float) + assert cycle_data["elapsed_seconds"] >= 0 + + @pytest.mark.asyncio + async def test_reviewer_error_treated_as_rejection(self, tmp_path: Path): + """Test that reviewer agent errors are treated as rejections.""" + import sys + + sys.path.insert(0, str(Path(__file__).parents[3] / "containers")) + + review_md = tmp_path / "review.md" + review_md.write_text("---\nmax_retries: 1\n---\nReview") + + with patch("entrypoint.run_reviewer_agent") as mock_reviewer: + mock_reviewer.side_effect = RuntimeError("API Error") + + from entrypoint import run_review_loop + + result = await run_review_loop( + workspace=tmp_path, + task_key="TEST-123", + task_summary="Test", + task_description="Desc", + guardrails="", + skill_name="test-skill", + review_md_path=review_md, + ) + + # Should still exit successfully after max retries + assert result is True + + # Check that rejection was recorded + cycle_file = ( + tmp_path / ".forge" / "reviews" / "TEST-123__test-skill" / "review_cycle_1.json" + ) + cycle_data = json.loads(cycle_file.read_text()) + assert cycle_data["verdict"] == "rejected" + + +# --------------------------------------------------------------------------- +# Test main function review loop integration +# --------------------------------------------------------------------------- + + +class TestMainReviewLoopIntegration: + def test_skips_review_when_no_skill_name(self, tmp_path: Path, monkeypatch): + """Test that review loop is skipped when no skill_name provided (SC-010).""" + import sys + + sys.path.insert(0, str(Path(__file__).parents[3] / "containers")) + + # Create task file without skill_name + task_file = tmp_path / "task.json" + task_file.write_text( + json.dumps( + { + "task_key": "TEST-123", + "summary": "Test task", + "description": "Test description", + } + ) + ) + + # Create workspace + workspace = tmp_path / "workspace" + workspace.mkdir() + + monkeypatch.setenv("FORGE_SYSTEM_PROMPT_TEMPLATE", "Test prompt {task_key}") + monkeypatch.chdir(workspace) + + def _close_and_return_true(coro): + coro.close() + return True + + with ( + patch("entrypoint.asyncio.run") as mock_asyncio_run, + patch("entrypoint.configure_git"), + patch("entrypoint.subprocess.run") as mock_subprocess, + ): + mock_asyncio_run.side_effect = _close_and_return_true + mock_subprocess.return_value = MagicMock(returncode=0) + + from entrypoint import main + + with pytest.raises(SystemExit) as exc_info: + sys.argv = [ + "entrypoint.py", + "--task-file", + str(task_file), + "--workspace", + str(workspace), + ] + main() + + # Should exit successfully + assert exc_info.value.code == 0 + + def test_skips_review_when_no_review_md(self, tmp_path: Path, monkeypatch): + """Test that review loop is skipped when review.md doesn't exist (SC-010).""" + import sys + + sys.path.insert(0, str(Path(__file__).parents[3] / "containers")) + + # Create task file with skill_name + task_file = tmp_path / "task.json" + task_file.write_text( + json.dumps( + { + "task_key": "TEST-123", + "summary": "Test task", + "description": "Test description", + "skill_name": "nonexistent-skill", + } + ) + ) + + # Create workspace + workspace = tmp_path / "workspace" + workspace.mkdir() + + monkeypatch.setenv("FORGE_SYSTEM_PROMPT_TEMPLATE", "Test prompt {task_key}") + monkeypatch.chdir(workspace) + + def _close_and_return_true(coro): + coro.close() + return True + + with ( + patch("entrypoint.asyncio.run") as mock_asyncio_run, + patch("entrypoint.configure_git"), + patch("entrypoint.subprocess.run") as mock_subprocess, + patch("entrypoint.detect_review_md") as mock_detect, + ): + mock_asyncio_run.side_effect = _close_and_return_true + mock_subprocess.return_value = MagicMock(returncode=0) + mock_detect.return_value = None # No review.md found + + from entrypoint import main + + with pytest.raises(SystemExit) as exc_info: + sys.argv = [ + "entrypoint.py", + "--task-file", + str(task_file), + "--workspace", + str(workspace), + ] + main() + + # Should exit successfully without running review loop + assert exc_info.value.code == 0 + # run_review_loop should not have been called + # (only run_agent_task via asyncio.run) + assert mock_asyncio_run.call_count == 1 + + def test_runs_review_when_review_md_exists(self, tmp_path: Path, monkeypatch): + """Test that review loop runs when review.md exists (SC-001).""" + import sys + + sys.path.insert(0, str(Path(__file__).parents[3] / "containers")) + + # Create task file with skill_name + task_file = tmp_path / "task.json" + task_file.write_text( + json.dumps( + { + "task_key": "TEST-123", + "summary": "Test task", + "description": "Test description", + "skill_name": "test-skill", + } + ) + ) + + # Create workspace + workspace = tmp_path / "workspace" + workspace.mkdir() + + # Create review.md + review_md = tmp_path / "review.md" + review_md.write_text("---\nmax_retries: 1\n---\nReview instructions") + + monkeypatch.setenv("FORGE_SYSTEM_PROMPT_TEMPLATE", "Test prompt {task_key}") + monkeypatch.chdir(workspace) + + asyncio_call_count = 0 + + def mock_asyncio_side_effect(coro): + nonlocal asyncio_call_count + asyncio_call_count += 1 + coro.close() + return True + + with ( + patch("entrypoint.asyncio.run") as mock_asyncio_run, + patch("entrypoint.configure_git"), + patch("entrypoint.subprocess.run") as mock_subprocess, + patch("entrypoint.detect_review_md") as mock_detect, + ): + mock_asyncio_run.side_effect = mock_asyncio_side_effect + mock_subprocess.return_value = MagicMock(returncode=0) + mock_detect.return_value = review_md + + from entrypoint import main + + with pytest.raises(SystemExit) as exc_info: + sys.argv = [ + "entrypoint.py", + "--task-file", + str(task_file), + "--workspace", + str(workspace), + ] + main() + + assert exc_info.value.code == 0 + # Should have called asyncio.run twice: once for run_agent_task, once for run_review_loop + assert asyncio_call_count == 2 + + +# --------------------------------------------------------------------------- +# Test _discover_skill_paths +# --------------------------------------------------------------------------- + + +class TestDiscoverSkillPaths: + """Tests for _discover_skill_paths function.""" + + def test_parses_comma_separated_env_var(self, tmp_path: Path, monkeypatch): + """Test parsing AGENT_SKILL_PATHS env var (comma-separated).""" + monkeypatch.setenv("AGENT_SKILL_PATHS", "/path/a/,/path/b/,/path/c/") + + from entrypoint import _discover_skill_paths + + result = _discover_skill_paths(tmp_path) + assert "/path/a/" in result + assert "/path/b/" in result + assert "/path/c/" in result + + def test_trailing_slash_added_when_missing(self, tmp_path: Path, monkeypatch): + """Test trailing slash is added when missing.""" + monkeypatch.setenv("AGENT_SKILL_PATHS", "/path/a,/path/b/") + + from entrypoint import _discover_skill_paths + + result = _discover_skill_paths(tmp_path) + assert "/path/a/" in result + assert "/path/b/" in result + + def test_auto_discovers_claude_skills_dir(self, tmp_path: Path, monkeypatch): + """Test auto-discovery of .claude/skills workspace dir.""" + monkeypatch.delenv("AGENT_SKILL_PATHS", raising=False) + + # Create .claude/skills directory + (tmp_path / ".claude" / "skills").mkdir(parents=True) + + from entrypoint import _discover_skill_paths + + result = _discover_skill_paths(tmp_path) + assert f"{tmp_path / '.claude' / 'skills'}/" in result + + def test_auto_discovers_agents_skills_dir(self, tmp_path: Path, monkeypatch): + """Test auto-discovery of .agents/skills workspace dir.""" + monkeypatch.delenv("AGENT_SKILL_PATHS", raising=False) + + # Create .agents/skills directory + (tmp_path / ".agents" / "skills").mkdir(parents=True) + + from entrypoint import _discover_skill_paths + + result = _discover_skill_paths(tmp_path) + assert f"{tmp_path / '.agents' / 'skills'}/" in result + + def test_empty_env_returns_only_auto_discovered(self, tmp_path: Path, monkeypatch): + """Test empty env var returns only auto-discovered paths.""" + monkeypatch.setenv("AGENT_SKILL_PATHS", "") + + # Create one auto-discoverable directory + (tmp_path / ".claude" / "skills").mkdir(parents=True) + + from entrypoint import _discover_skill_paths + + result = _discover_skill_paths(tmp_path) + # Should only contain auto-discovered path + assert len(result) == 1 + assert f"{tmp_path / '.claude' / 'skills'}/" in result + + def test_deduplication(self, tmp_path: Path, monkeypatch): + """Test deduplication between env var and auto-discovered paths.""" + # Create .claude/skills directory + claude_skills = tmp_path / ".claude" / "skills" + claude_skills.mkdir(parents=True) + + # Set env var to include the same path that would be auto-discovered + monkeypatch.setenv("AGENT_SKILL_PATHS", f"{claude_skills}/") + + from entrypoint import _discover_skill_paths + + result = _discover_skill_paths(tmp_path) + # Should not have duplicates + assert result.count(f"{claude_skills}/") == 1 + + +# --------------------------------------------------------------------------- +# Test _fallback_commit +# --------------------------------------------------------------------------- + + +class TestFallbackCommit: + """Tests for _fallback_commit function.""" + + def test_calls_git_commit_when_git_repo(self, tmp_path: Path, monkeypatch): + """Test it calls git_commit when workspace is a git repo.""" + from unittest.mock import MagicMock + + import entrypoint + + # Mock subprocess.run to return is_git_repo=True + mock_subprocess_run = MagicMock() + mock_subprocess_run.return_value = MagicMock(returncode=0) + monkeypatch.setattr(entrypoint, "subprocess", MagicMock(run=mock_subprocess_run)) + + # Mock git_commit to succeed + mock_git_commit = MagicMock(return_value=True) + monkeypatch.setattr(entrypoint, "git_commit", mock_git_commit) + + entrypoint._fallback_commit(tmp_path, "TEST-1", "Test summary") + + mock_git_commit.assert_called_once() + call_args = mock_git_commit.call_args + assert call_args[0][0] == tmp_path + assert "TEST-1" in call_args[0][1] + assert "Test summary" in call_args[0][1] + + def test_skips_when_not_git_repo(self, tmp_path: Path, monkeypatch): + """Test it skips when workspace is NOT a git repo.""" + from unittest.mock import MagicMock + + import entrypoint + + # Mock subprocess.run to return is_git_repo=False + mock_subprocess_run = MagicMock() + mock_subprocess_run.return_value = MagicMock(returncode=128) + monkeypatch.setattr(entrypoint, "subprocess", MagicMock(run=mock_subprocess_run)) + + mock_git_commit = MagicMock() + monkeypatch.setattr(entrypoint, "git_commit", mock_git_commit) + + entrypoint._fallback_commit(tmp_path, "TEST-1", "Test summary") + + mock_git_commit.assert_not_called() + + def test_exits_when_git_commit_fails(self, tmp_path: Path, monkeypatch): + """Test it calls sys.exit(EXIT_TASK_FAILED) when git_commit fails.""" + from unittest.mock import MagicMock + + import entrypoint + + # Mock subprocess.run to return is_git_repo=True + mock_subprocess_run = MagicMock() + mock_subprocess_run.return_value = MagicMock(returncode=0) + monkeypatch.setattr(entrypoint, "subprocess", MagicMock(run=mock_subprocess_run)) + + # Mock git_commit to fail + monkeypatch.setattr(entrypoint, "git_commit", MagicMock(return_value=False)) + + with pytest.raises(SystemExit) as exc_info: + entrypoint._fallback_commit(tmp_path, "TEST-1", "Test summary") + + assert exc_info.value.code == 1 # EXIT_TASK_FAILED + + +# --------------------------------------------------------------------------- +# Test _setup_langfuse_tracing +# --------------------------------------------------------------------------- + + +class TestSetupLangfuseTracing: + """Tests for _setup_langfuse_tracing function.""" + + def test_returns_config_with_callbacks_when_key_set(self, monkeypatch): + """Test returns (config_with_callbacks, True) when LANGFUSE_PUBLIC_KEY is set.""" + monkeypatch.setenv("LANGFUSE_PUBLIC_KEY", "pk-test-123") + + from entrypoint import _setup_langfuse_tracing + + with patch("entrypoint.CallbackHandler", create=True) as mock_handler_cls: + # Use importlib to make langfuse.langchain.CallbackHandler importable + mock_handler = MagicMock() + mock_handler_cls.return_value = mock_handler + + with patch.dict( + "sys.modules", + { + "langfuse": MagicMock(), + "langfuse.langchain": MagicMock(CallbackHandler=mock_handler_cls), + }, + ): + config, enabled = _setup_langfuse_tracing("TEST-1", {}) + + assert enabled is True + assert "callbacks" in config + assert len(config["callbacks"]) == 1 + + def test_returns_empty_when_key_not_set(self, monkeypatch): + """Test returns ({}, False) when LANGFUSE_PUBLIC_KEY is not set.""" + monkeypatch.delenv("LANGFUSE_PUBLIC_KEY", raising=False) + + from entrypoint import _setup_langfuse_tracing + + config, enabled = _setup_langfuse_tracing("TEST-1", {}) + + assert enabled is False + assert config == {} + + def test_returns_empty_when_import_fails(self, monkeypatch): + """Test returns ({}, False) when langfuse import fails.""" + monkeypatch.setenv("LANGFUSE_PUBLIC_KEY", "pk-test-123") + + from entrypoint import _setup_langfuse_tracing + + with patch.dict("sys.modules", {"langfuse": None, "langfuse.langchain": None}): + config, enabled = _setup_langfuse_tracing("TEST-1", {}) + + assert enabled is False + assert config == {} + + +# --------------------------------------------------------------------------- +# Test _parse_task_config +# --------------------------------------------------------------------------- + + +class TestParseTaskConfig: + """Tests for _parse_task_config function.""" + + def test_cli_args_branch(self): + """Test CLI args branch (--task-summary + --task-description).""" + from entrypoint import _parse_task_config + + args = MagicMock() + args.task_file = None + args.task_summary = "CLI summary" + args.task_description = "CLI description" + + result = _parse_task_config(args) + + assert result["task_key"] == "UNKNOWN" + assert result["task_summary"] == "CLI summary" + assert result["task_description"] == "CLI description" + assert result["skill_name"] == "" + assert result["previous_task_keys"] == [] + assert result["trace_context"] == {} + + def test_sys_exit_when_no_args_provided(self): + """Test sys.exit when neither task-file nor CLI args provided.""" + from entrypoint import _parse_task_config + + args = MagicMock() + args.task_file = None + args.task_summary = None + args.task_description = None + + with pytest.raises(SystemExit) as exc_info: + _parse_task_config(args) + + assert exc_info.value.code == 3 # EXIT_CONFIG_ERROR + + def test_trace_context_type_guard_non_dict(self, tmp_path: Path): + """Test trace_context type guard (non-dict becomes {}).""" + from entrypoint import _parse_task_config + + task_file = tmp_path / "task.json" + task_file.write_text( + json.dumps( + { + "task_key": "TEST-1", + "summary": "Test", + "description": "Test desc", + "trace_context": "not-a-dict", + } + ) + ) + + args = MagicMock() + args.task_file = task_file + args.task_summary = None + args.task_description = None + + result = _parse_task_config(args) + + assert result["trace_context"] == {} + + +# --------------------------------------------------------------------------- +# Test run_reviewer_agent with empty messages +# --------------------------------------------------------------------------- + + +class TestRunReviewerAgentEmptyMessages: + """Tests for run_reviewer_agent edge case with empty messages.""" + + @pytest.mark.asyncio + async def test_empty_messages_returns_empty_string(self, tmp_path: Path): + """Test that empty messages list returns empty string.""" + from entrypoint import run_reviewer_agent + + mock_agent = MagicMock() + mock_agent.ainvoke = AsyncMock(return_value={"messages": []}) + mock_create = MagicMock(return_value=mock_agent) + + with ( + patch("entrypoint._create_llm_model", return_value=("test-model", MagicMock())), + patch("deepagents.create_deep_agent", mock_create), + patch("deepagents.backends.LocalShellBackend"), + ): + result = await run_reviewer_agent( + workspace=tmp_path, + review_instructions="Check for bugs", + task_key="TEST-123", + ) + + assert result == "" + + +# --------------------------------------------------------------------------- +# Test _print_review_progress (SC-011) +# --------------------------------------------------------------------------- diff --git a/tests/unit/containers/test_review.py b/tests/unit/containers/test_review.py new file mode 100644 index 00000000..3056a765 --- /dev/null +++ b/tests/unit/containers/test_review.py @@ -0,0 +1,1144 @@ +"""Unit tests for containers.review module.""" + +import json +from pathlib import Path + +import pytest +from review import ( + DEFAULT_MAX_RETRIES, + ENV_MAX_RETRIES, + ReviewConfig, + ReviewCycleData, + Verdict, + detect_review_md, + find_skill_file, + parse_review_config, + parse_verdict, + review_cycle_dir_name, + write_cycle_file, +) + +# --------------------------------------------------------------------------- +# Verdict enum tests +# --------------------------------------------------------------------------- + + +class TestVerdict: + def test_approved_value(self): + assert Verdict.APPROVED == "approved" + assert Verdict.APPROVED.value == "approved" + + def test_rejected_value(self): + assert Verdict.REJECTED == "rejected" + assert Verdict.REJECTED.value == "rejected" + + def test_is_str_enum(self): + # StrEnum values can be used directly as strings + assert f"Verdict: {Verdict.APPROVED}" == "Verdict: approved" + + +# --------------------------------------------------------------------------- +# ReviewConfig dataclass tests +# --------------------------------------------------------------------------- + + +class TestReviewConfig: + def test_default_values(self): + config = ReviewConfig() + assert config.max_retries == DEFAULT_MAX_RETRIES + assert config.instructions == "" + + def test_custom_values(self): + config = ReviewConfig(max_retries=5, instructions="Check for bugs") + assert config.max_retries == 5 + assert config.instructions == "Check for bugs" + + def test_default_max_retries_is_3(self): + assert DEFAULT_MAX_RETRIES == 3 + + +# --------------------------------------------------------------------------- +# ReviewCycleData dataclass tests +# --------------------------------------------------------------------------- + + +class TestReviewCycleData: + def test_all_fields_required(self): + data = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="approved", + feedback="Looks good!", + skill="code-review", + elapsed_seconds=12.5, + timestamp="2024-01-15T10:30:00Z", + ) + assert data.cycle == 1 + assert data.max_cycles == 3 + assert data.verdict == "approved" + assert data.feedback == "Looks good!" + assert data.skill == "code-review" + assert data.elapsed_seconds == 12.5 + assert data.timestamp == "2024-01-15T10:30:00Z" + + def test_verdict_can_be_approved_or_rejected(self): + approved = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict=Verdict.APPROVED, + feedback="", + skill="test", + elapsed_seconds=1.0, + timestamp="2024-01-01T00:00:00Z", + ) + assert approved.verdict == "approved" + + rejected = ReviewCycleData( + cycle=2, + max_cycles=3, + verdict=Verdict.REJECTED, + feedback="Needs work", + skill="test", + elapsed_seconds=2.0, + timestamp="2024-01-01T00:00:00Z", + ) + assert rejected.verdict == "rejected" + + +# --------------------------------------------------------------------------- +# parse_review_config tests +# --------------------------------------------------------------------------- + + +class TestParseReviewConfig: + """Tests for parse_review_config function.""" + + def test_parse_valid_frontmatter(self, tmp_path: Path): + review_md = tmp_path / "review.md" + review_md.write_text( + """\ +--- +max_retries: 5 +--- +Review the code for security issues. +""" + ) + config = parse_review_config(review_md) + assert config.max_retries == 5 + assert config.instructions == "Review the code for security issues." + + def test_parse_only_instructions_no_frontmatter(self, tmp_path: Path): + review_md = tmp_path / "review.md" + review_md.write_text("Just some instructions, no frontmatter.") + + config = parse_review_config(review_md) + assert config.max_retries == DEFAULT_MAX_RETRIES + assert config.instructions == "Just some instructions, no frontmatter." + + def test_parse_empty_frontmatter(self, tmp_path: Path): + review_md = tmp_path / "review.md" + review_md.write_text( + """\ +--- +--- +Instructions after empty frontmatter. +""" + ) + config = parse_review_config(review_md) + assert config.max_retries == DEFAULT_MAX_RETRIES + assert config.instructions == "Instructions after empty frontmatter." + + def test_file_not_found_returns_defaults(self, tmp_path: Path): + review_md = tmp_path / "nonexistent.md" + config = parse_review_config(review_md) + assert config.max_retries == DEFAULT_MAX_RETRIES + assert config.instructions == "" + + def test_malformed_yaml_logs_warning_and_returns_defaults( + self, tmp_path: Path, caplog: pytest.LogCaptureFixture + ): + review_md = tmp_path / "review.md" + review_md.write_text( + """\ +--- +max_retries: [invalid yaml +--- +Some instructions. +""" + ) + config = parse_review_config(review_md) + assert config.max_retries == DEFAULT_MAX_RETRIES + assert config.instructions == "Some instructions." + assert "Malformed YAML" in caplog.text + + def test_env_var_fallback_when_no_frontmatter_value( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ): + monkeypatch.setenv(ENV_MAX_RETRIES, "7") + + review_md = tmp_path / "review.md" + review_md.write_text("No frontmatter, just instructions.") + + config = parse_review_config(review_md) + assert config.max_retries == 7 + assert config.instructions == "No frontmatter, just instructions." + + def test_frontmatter_overrides_env_var(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch): + monkeypatch.setenv(ENV_MAX_RETRIES, "7") + + review_md = tmp_path / "review.md" + review_md.write_text( + """\ +--- +max_retries: 2 +--- +Instructions here. +""" + ) + config = parse_review_config(review_md) + assert config.max_retries == 2 + + def test_invalid_env_var_is_ignored( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture + ): + monkeypatch.setenv(ENV_MAX_RETRIES, "not-a-number") + + review_md = tmp_path / "review.md" + review_md.write_text("Instructions only.") + + config = parse_review_config(review_md) + assert config.max_retries == DEFAULT_MAX_RETRIES + assert "Invalid AUTO_REVIEW_MAX_RETRIES" in caplog.text + + def test_non_integer_max_retries_in_frontmatter_uses_default( + self, tmp_path: Path, caplog: pytest.LogCaptureFixture + ): + review_md = tmp_path / "review.md" + review_md.write_text( + """\ +--- +max_retries: "five" +--- +Instructions. +""" + ) + config = parse_review_config(review_md) + assert config.max_retries == DEFAULT_MAX_RETRIES + assert "Invalid max_retries" in caplog.text + + def test_multiline_instructions(self, tmp_path: Path): + review_md = tmp_path / "review.md" + review_md.write_text( + """\ +--- +max_retries: 4 +--- +Line one. +Line two. +Line three. +""" + ) + config = parse_review_config(review_md) + assert config.max_retries == 4 + assert "Line one." in config.instructions + assert "Line two." in config.instructions + assert "Line three." in config.instructions + + def test_frontmatter_with_extra_fields_ignored(self, tmp_path: Path): + review_md = tmp_path / "review.md" + review_md.write_text( + """\ +--- +max_retries: 6 +author: test +some_other_field: value +--- +Instructions. +""" + ) + config = parse_review_config(review_md) + assert config.max_retries == 6 + assert config.instructions == "Instructions." + + def test_env_var_fallback_for_file_not_found( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ): + monkeypatch.setenv(ENV_MAX_RETRIES, "10") + + review_md = tmp_path / "nonexistent.md" + config = parse_review_config(review_md) + assert config.max_retries == 10 + assert config.instructions == "" + + def test_env_var_fallback_for_malformed_yaml( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ): + monkeypatch.setenv(ENV_MAX_RETRIES, "8") + + review_md = tmp_path / "review.md" + review_md.write_text( + """\ +--- +: invalid: yaml: +--- +Some instructions. +""" + ) + config = parse_review_config(review_md) + assert config.max_retries == 8 + assert config.instructions == "Some instructions." + + def test_no_trailing_newline_after_frontmatter(self, tmp_path: Path): + review_md = tmp_path / "review.md" + review_md.write_text("---\nmax_retries: 3\n---\nInstructions.") + config = parse_review_config(review_md) + assert config.max_retries == 3 + assert config.instructions == "Instructions." + + def test_whitespace_handling_in_instructions(self, tmp_path: Path): + review_md = tmp_path / "review.md" + review_md.write_text( + """\ +--- +max_retries: 1 +--- + + Indented instructions with leading/trailing whitespace. + +""" + ) + config = parse_review_config(review_md) + # Instructions should be stripped + assert config.instructions == "Indented instructions with leading/trailing whitespace." + + +# --------------------------------------------------------------------------- +# detect_review_md tests +# --------------------------------------------------------------------------- + + +class TestFindSkillFile: + """Tests for find_skill_file — generic skill file search across mounted paths.""" + + def test_finds_file_in_single_path(self, tmp_path: Path): + skill_dir = tmp_path / "skill_0" / "implement-task" + skill_dir.mkdir(parents=True) + review_file = skill_dir / "review.md" + review_file.write_text("Review instructions") + + result = find_skill_file("implement-task", "review.md", [str(tmp_path / "skill_0")]) + + assert result == review_file + + def test_later_path_overrides_earlier(self, tmp_path: Path): + default_dir = tmp_path / "skill_0" / "implement-task" + default_dir.mkdir(parents=True) + (default_dir / "review.md").write_text("Default review") + + project_dir = tmp_path / "skill_1" / "implement-task" + project_dir.mkdir(parents=True) + project_file = project_dir / "review.md" + project_file.write_text("Project override") + + result = find_skill_file( + "implement-task", + "review.md", + [str(tmp_path / "skill_0"), str(tmp_path / "skill_1")], + ) + + assert result == project_file + assert result.read_text() == "Project override" + + def test_falls_back_to_earlier_path(self, tmp_path: Path): + default_dir = tmp_path / "skill_0" / "implement-task" + default_dir.mkdir(parents=True) + default_file = default_dir / "review.md" + default_file.write_text("Default review") + + # skill_1 exists but has no review.md for this skill + (tmp_path / "skill_1").mkdir(parents=True) + + result = find_skill_file( + "implement-task", + "review.md", + [str(tmp_path / "skill_0"), str(tmp_path / "skill_1")], + ) + + assert result == default_file + + def test_returns_none_when_not_found(self, tmp_path: Path): + (tmp_path / "skill_0").mkdir(parents=True) + + result = find_skill_file( + "implement-task", + "review.md", + [str(tmp_path / "skill_0")], + ) + + assert result is None + + def test_returns_none_for_empty_paths(self): + result = find_skill_file("implement-task", "review.md", []) + assert result is None + + def test_reads_from_env_var(self, tmp_path: Path, monkeypatch): + skill_dir = tmp_path / "mounted" / "implement-task" + skill_dir.mkdir(parents=True) + skill_file = skill_dir / "SKILL.md" + skill_file.write_text("Skill content") + + monkeypatch.setenv("AGENT_SKILL_PATHS", str(tmp_path / "mounted")) + + result = find_skill_file("implement-task", "SKILL.md") + + assert result == skill_file + + def test_finds_skill_md(self, tmp_path: Path): + skill_dir = tmp_path / "skills" / "implement-task" + skill_dir.mkdir(parents=True) + skill_file = skill_dir / "SKILL.md" + skill_file.write_text("Skill instructions") + + result = find_skill_file("implement-task", "SKILL.md", [str(tmp_path / "skills")]) + + assert result == skill_file + + def test_ignores_directory_with_same_name(self, tmp_path: Path): + skill_dir = tmp_path / "skills" / "edge-skill" + skill_dir.mkdir(parents=True) + (skill_dir / "review.md").mkdir() # directory, not file + + result = find_skill_file("edge-skill", "review.md", [str(tmp_path / "skills")]) + + assert result is None + + def test_handles_trailing_slash_in_paths(self, tmp_path: Path): + skill_dir = tmp_path / "skill_0" / "my-skill" + skill_dir.mkdir(parents=True) + review_file = skill_dir / "review.md" + review_file.write_text("Review") + + result = find_skill_file("my-skill", "review.md", [str(tmp_path / "skill_0") + "/"]) + + assert result == review_file + + +class TestDetectReviewMd: + """Tests for detect_review_md — delegates to find_skill_file.""" + + def test_finds_review_md_via_skill_paths(self, tmp_path: Path): + skill_dir = tmp_path / "skill_0" / "implement-task" + skill_dir.mkdir(parents=True) + review_file = skill_dir / "review.md" + review_file.write_text("Review instructions") + + result = detect_review_md( + "implement-task", + skill_paths=[str(tmp_path / "skill_0")], + ) + + assert result == review_file + + def test_returns_none_when_not_found(self, tmp_path: Path): + (tmp_path / "skill_0").mkdir(parents=True) + + result = detect_review_md( + "implement-task", + skill_paths=[str(tmp_path / "skill_0")], + ) + + assert result is None + + def test_reads_env_var_when_no_skill_paths(self, tmp_path: Path, monkeypatch): + skill_dir = tmp_path / "mounted" / "implement-task" + skill_dir.mkdir(parents=True) + review_file = skill_dir / "review.md" + review_file.write_text("Review") + + monkeypatch.setenv("AGENT_SKILL_PATHS", str(tmp_path / "mounted")) + + result = detect_review_md("implement-task") + + assert result == review_file + + +# --------------------------------------------------------------------------- +# parse_verdict tests +# --------------------------------------------------------------------------- + + +class TestParseVerdict: + """Tests for parse_verdict function (SC-002).""" + + # ----- APPROVED marker tests ----- + + def test_approved_uppercase(self): + """APPROVED marker in uppercase returns (APPROVED, '').""" + result = parse_verdict("The code looks good. APPROVED") + assert result == (Verdict.APPROVED, "") + + def test_approved_lowercase(self): + """Case-insensitive: 'approved' returns (APPROVED, '').""" + result = parse_verdict("Code review complete. approved") + assert result == (Verdict.APPROVED, "") + + def test_approved_mixed_case(self): + """Case-insensitive: 'Approved' returns (APPROVED, '').""" + result = parse_verdict("All tests pass. Approved") + assert result == (Verdict.APPROVED, "") + + def test_approved_with_text_before_and_after(self): + """APPROVED marker in middle of text still returns (APPROVED, '').""" + result = parse_verdict("Summary: Code is clean. APPROVED. No further changes needed.") + assert result == (Verdict.APPROVED, "") + + def test_approved_at_start(self): + """APPROVED at start of text.""" + result = parse_verdict("APPROVED - code meets all requirements") + assert result == (Verdict.APPROVED, "") + + # ----- REJECTED marker tests ----- + + def test_rejected_uppercase(self): + """REJECTED marker returns (REJECTED, feedback).""" + result = parse_verdict("REJECTED: Code has security issues.") + assert result[0] == Verdict.REJECTED + assert result[1] == ": Code has security issues." + + def test_rejected_lowercase(self): + """Case-insensitive: 'rejected' returns (REJECTED, feedback).""" + result = parse_verdict("Code review result: rejected due to missing tests.") + assert result[0] == Verdict.REJECTED + assert result[1] == "due to missing tests." + + def test_rejected_mixed_case(self): + """Case-insensitive: 'Rejected' returns (REJECTED, feedback).""" + result = parse_verdict("Rejected - needs refactoring.") + assert result[0] == Verdict.REJECTED + assert result[1] == "- needs refactoring." + + def test_rejected_extracts_feedback_after_marker(self): + """Feedback is all text after REJECTED marker.""" + result = parse_verdict( + "Review: REJECTED\n\nPlease fix the following:\n1. Bug in line 42\n2. Missing docstring" + ) + assert result[0] == Verdict.REJECTED + assert "Please fix the following:" in result[1] + assert "Bug in line 42" in result[1] + assert "Missing docstring" in result[1] + + def test_rejected_feedback_is_stripped(self): + """Feedback text is stripped of leading/trailing whitespace.""" + result = parse_verdict("REJECTED \n\n Needs work. \n\n") + assert result[0] == Verdict.REJECTED + assert result[1] == "Needs work." + + def test_rejected_with_empty_feedback(self): + """SC-003: Empty feedback after REJECTED marker is handled correctly.""" + result = parse_verdict("REJECTED") + assert result[0] == Verdict.REJECTED + assert result[1] == "" + + def test_rejected_with_only_whitespace_feedback(self): + """Whitespace-only feedback after REJECTED is stripped to empty string.""" + result = parse_verdict("REJECTED \n\n \t \n") + assert result[0] == Verdict.REJECTED + assert result[1] == "" + + # ----- Neither marker present ----- + + def test_neither_marker_returns_rejected_with_error_message(self): + """Neither marker present returns (REJECTED, 'Verdict could not be parsed').""" + result = parse_verdict("This review output has no clear verdict.") + assert result == (Verdict.REJECTED, "Verdict could not be parsed") + + def test_empty_string_returns_rejected_with_error_message(self): + """Empty string returns (REJECTED, 'Verdict could not be parsed').""" + result = parse_verdict("") + assert result == (Verdict.REJECTED, "Verdict could not be parsed") + + def test_whitespace_only_returns_rejected_with_error_message(self): + """Whitespace-only string returns (REJECTED, 'Verdict could not be parsed').""" + result = parse_verdict(" \n\t\n ") + assert result == (Verdict.REJECTED, "Verdict could not be parsed") + + # ----- Both markers present ----- + + def test_both_markers_approved_first_wins(self): + """When both markers present, APPROVED first wins.""" + result = parse_verdict("Code is APPROVED, not REJECTED because tests pass.") + assert result == (Verdict.APPROVED, "") + + def test_both_markers_rejected_first_wins(self): + """When both markers present, REJECTED first wins if it comes first.""" + result = parse_verdict( + "This code is REJECTED. It would have been APPROVED if tests passed." + ) + assert result[0] == Verdict.REJECTED + assert "It would have been APPROVED if tests passed." in result[1] + + def test_both_markers_same_position_edge_case(self): + """Edge case: if somehow both start at same position, check behavior. + + In practice this can't happen since they're different strings, + but we verify APPROVED is checked first per spec. + """ + # This tests the logic: APPROVED found, REJECTED found, but APPROVED position is smaller + text = "APPROVED followed by REJECTED" + result = parse_verdict(text) + assert result == (Verdict.APPROVED, "") + + # ----- Partial matches should not be detected ----- + + def test_approved_as_word_in_middle(self): + """APPROVED as substring in longer word is NOT detected (word boundary matching).""" + # Word-boundary regex (\bAPPROVED\b) prevents false positives like "PREAPPROVED" + result = parse_verdict("This is PREAPPROVED for the next phase") + assert result == (Verdict.REJECTED, "Verdict could not be parsed") + + def test_rejected_as_word_in_middle(self): + """REJECTED as substring is still detected (per current impl).""" + result = parse_verdict("Previously rejected items were fixed") + # "rejected" is found in the middle + assert result[0] == Verdict.REJECTED + + # ----- Complex real-world scenarios ----- + + def test_real_world_approved_review(self): + """Real-world style APPROVED review.""" + review = """ +## Code Review Summary + +The implementation looks good and follows the coding standards. +All tests pass and documentation is adequate. + +**Verdict: APPROVED** + +No further changes required. +""" + result = parse_verdict(review) + assert result == (Verdict.APPROVED, "") + + def test_real_world_rejected_review(self): + """Real-world style REJECTED review with detailed feedback.""" + review = """ +## Code Review Summary + +The implementation has several issues that need to be addressed. + +**Verdict: REJECTED** + +### Issues Found: +1. Missing error handling in `parse_data()` function +2. No unit tests for edge cases +3. Documentation needs to be updated + +### Recommendations: +- Add try/except blocks for file operations +- Add tests for empty input and malformed data +""" + result = parse_verdict(review) + assert result[0] == Verdict.REJECTED + assert "Missing error handling" in result[1] + assert "No unit tests for edge cases" in result[1] + assert "Recommendations:" in result[1] + + def test_multiline_approved(self): + """APPROVED marker on its own line.""" + review = """ +Review complete. + +APPROVED + +Ship it! +""" + result = parse_verdict(review) + assert result == (Verdict.APPROVED, "") + + +# --------------------------------------------------------------------------- +# review_cycle_dir_name tests +# --------------------------------------------------------------------------- + + +class TestReviewCycleDirName: + """Tests for review_cycle_dir_name function.""" + + def test_returns_task_key_double_underscore_skill_name(self): + """Test it returns '{task_key}__{skill_name}' format.""" + result = review_cycle_dir_name("AISOS-2126", "implement-task") + assert result == "AISOS-2126__implement-task" + + def test_with_different_inputs(self): + """Test with different task_key and skill_name values.""" + result = review_cycle_dir_name("PROJ-42", "local-code-review") + assert result == "PROJ-42__local-code-review" + + +# --------------------------------------------------------------------------- +# write_cycle_file tests +# --------------------------------------------------------------------------- + + +class TestWriteCycleFile: + """Tests for write_cycle_file function (SC-007).""" + + def test_creates_step_directory(self, tmp_path: Path): + """Creates .forge/{step_name}/ directory if it doesn't exist.""" + workspace = tmp_path / "workspace" + workspace.mkdir() + + cycle_data = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="approved", + feedback="", + skill="local-code-review", + elapsed_seconds=15.5, + timestamp="2024-01-15T10:30:00Z", + ) + + write_cycle_file(workspace, "TEST-1", "implement", cycle_data) + + step_dir = workspace / ".forge" / "reviews" / "TEST-1__implement" + assert step_dir.exists() + assert step_dir.is_dir() + + def test_creates_nested_forge_and_step_directories(self, tmp_path: Path): + """Creates both .forge/ and step subdirectory when neither exists.""" + workspace = tmp_path / "workspace" + workspace.mkdir() + + cycle_data = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="approved", + feedback="", + skill="code-review", + elapsed_seconds=10.0, + timestamp="2024-01-15T10:30:00Z", + ) + + write_cycle_file(workspace, "TEST-1", "my-step", cycle_data) + + assert (workspace / ".forge").exists() + assert (workspace / ".forge" / "reviews" / "TEST-1__my-step").exists() + + def test_writes_review_cycle_n_json(self, tmp_path: Path): + """File written to .forge/{step_name}/review_cycle_N.json.""" + workspace = tmp_path / "workspace" + workspace.mkdir() + + cycle_data = ReviewCycleData( + cycle=2, + max_cycles=5, + verdict="rejected", + feedback="Needs work", + skill="test-skill", + elapsed_seconds=20.0, + timestamp="2024-01-15T10:30:00Z", + ) + + write_cycle_file(workspace, "TEST-1", "step-name", cycle_data) + + output_file = workspace / ".forge" / "reviews" / "TEST-1__step-name" / "review_cycle_2.json" + assert output_file.exists() + assert output_file.is_file() + + def test_json_contains_all_required_fields(self, tmp_path: Path): + """SC-007: JSON contains all ReviewCycleData fields.""" + workspace = tmp_path / "workspace" + workspace.mkdir() + + cycle_data = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="approved", + feedback="Looks good!", + skill="local-code-review", + elapsed_seconds=12.5, + timestamp="2024-01-15T10:30:00Z", + ) + + write_cycle_file(workspace, "TEST-1", "implement", cycle_data) + + output_file = workspace / ".forge" / "reviews" / "TEST-1__implement" / "review_cycle_1.json" + data = json.loads(output_file.read_text(encoding="utf-8")) + + assert data["cycle"] == 1 + assert data["max_cycles"] == 3 + assert data["verdict"] == "approved" + assert data["feedback"] == "Looks good!" + assert data["skill"] == "local-code-review" + assert data["elapsed_seconds"] == 12.5 + assert data["timestamp"] == "2024-01-15T10:30:00Z" + + def test_timestamp_iso_8601_utc_format(self, tmp_path: Path): + """Timestamp is ISO 8601 UTC format.""" + workspace = tmp_path / "workspace" + workspace.mkdir() + + # ISO 8601 UTC timestamp + cycle_data = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="approved", + feedback="", + skill="test", + elapsed_seconds=5.0, + timestamp="2024-06-20T14:30:45Z", + ) + + write_cycle_file(workspace, "TEST-1", "step", cycle_data) + + output_file = workspace / ".forge" / "reviews" / "TEST-1__step" / "review_cycle_1.json" + data = json.loads(output_file.read_text(encoding="utf-8")) + + # Verify timestamp format preserved + assert data["timestamp"] == "2024-06-20T14:30:45Z" + + def test_verdict_lowercase_approved(self, tmp_path: Path): + """Verdict is lowercase string 'approved'.""" + workspace = tmp_path / "workspace" + workspace.mkdir() + + cycle_data = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="approved", + feedback="", + skill="test", + elapsed_seconds=5.0, + timestamp="2024-01-15T10:30:00Z", + ) + + write_cycle_file(workspace, "TEST-1", "step", cycle_data) + + output_file = workspace / ".forge" / "reviews" / "TEST-1__step" / "review_cycle_1.json" + data = json.loads(output_file.read_text(encoding="utf-8")) + + assert data["verdict"] == "approved" + assert data["verdict"].islower() + + def test_verdict_lowercase_rejected(self, tmp_path: Path): + """Verdict is lowercase string 'rejected'.""" + workspace = tmp_path / "workspace" + workspace.mkdir() + + cycle_data = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="rejected", + feedback="Fix the bugs", + skill="test", + elapsed_seconds=8.0, + timestamp="2024-01-15T10:30:00Z", + ) + + write_cycle_file(workspace, "TEST-1", "step", cycle_data) + + output_file = workspace / ".forge" / "reviews" / "TEST-1__step" / "review_cycle_1.json" + data = json.loads(output_file.read_text(encoding="utf-8")) + + assert data["verdict"] == "rejected" + assert data["verdict"].islower() + + def test_verdict_enum_converted_to_lowercase(self, tmp_path: Path): + """Verdict.APPROVED enum is converted to lowercase string.""" + workspace = tmp_path / "workspace" + workspace.mkdir() + + cycle_data = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict=Verdict.APPROVED, + feedback="", + skill="test", + elapsed_seconds=5.0, + timestamp="2024-01-15T10:30:00Z", + ) + + write_cycle_file(workspace, "TEST-1", "step", cycle_data) + + output_file = workspace / ".forge" / "reviews" / "TEST-1__step" / "review_cycle_1.json" + data = json.loads(output_file.read_text(encoding="utf-8")) + + assert data["verdict"] == "approved" + + def test_verdict_enum_rejected_converted_to_lowercase(self, tmp_path: Path): + """Verdict.REJECTED enum is converted to lowercase string.""" + workspace = tmp_path / "workspace" + workspace.mkdir() + + cycle_data = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict=Verdict.REJECTED, + feedback="Needs work", + skill="test", + elapsed_seconds=5.0, + timestamp="2024-01-15T10:30:00Z", + ) + + write_cycle_file(workspace, "TEST-1", "step", cycle_data) + + output_file = workspace / ".forge" / "reviews" / "TEST-1__step" / "review_cycle_1.json" + data = json.loads(output_file.read_text(encoding="utf-8")) + + assert data["verdict"] == "rejected" + + def test_multiple_cycles_written_separately(self, tmp_path: Path): + """Multiple cycles are written to separate files.""" + workspace = tmp_path / "workspace" + workspace.mkdir() + + cycle1 = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="rejected", + feedback="First attempt needs work", + skill="code-review", + elapsed_seconds=10.0, + timestamp="2024-01-15T10:30:00Z", + ) + cycle2 = ReviewCycleData( + cycle=2, + max_cycles=3, + verdict="rejected", + feedback="Still has issues", + skill="code-review", + elapsed_seconds=12.0, + timestamp="2024-01-15T10:35:00Z", + ) + cycle3 = ReviewCycleData( + cycle=3, + max_cycles=3, + verdict="approved", + feedback="", + skill="code-review", + elapsed_seconds=8.0, + timestamp="2024-01-15T10:40:00Z", + ) + + write_cycle_file(workspace, "TEST-1", "impl", cycle1) + write_cycle_file(workspace, "TEST-1", "impl", cycle2) + write_cycle_file(workspace, "TEST-1", "impl", cycle3) + + # All three files should exist + assert (workspace / ".forge" / "reviews" / "TEST-1__impl" / "review_cycle_1.json").exists() + assert (workspace / ".forge" / "reviews" / "TEST-1__impl" / "review_cycle_2.json").exists() + assert (workspace / ".forge" / "reviews" / "TEST-1__impl" / "review_cycle_3.json").exists() + + # Verify content of each + data1 = json.loads( + (workspace / ".forge" / "reviews" / "TEST-1__impl" / "review_cycle_1.json").read_text() + ) + data2 = json.loads( + (workspace / ".forge" / "reviews" / "TEST-1__impl" / "review_cycle_2.json").read_text() + ) + data3 = json.loads( + (workspace / ".forge" / "reviews" / "TEST-1__impl" / "review_cycle_3.json").read_text() + ) + + assert data1["verdict"] == "rejected" + assert data2["verdict"] == "rejected" + assert data3["verdict"] == "approved" + + def test_json_format_is_pretty_printed(self, tmp_path: Path): + """JSON is formatted with indentation for readability.""" + workspace = tmp_path / "workspace" + workspace.mkdir() + + cycle_data = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="approved", + feedback="", + skill="test", + elapsed_seconds=5.0, + timestamp="2024-01-15T10:30:00Z", + ) + + write_cycle_file(workspace, "TEST-1", "step", cycle_data) + + output_file = workspace / ".forge" / "reviews" / "TEST-1__step" / "review_cycle_1.json" + content = output_file.read_text(encoding="utf-8") + + # Should have newlines (pretty printed) + assert "\n" in content + # Should have indentation + assert " " in content + + def test_overwrites_existing_file(self, tmp_path: Path): + """Overwrites existing cycle file if run again with same cycle number.""" + workspace = tmp_path / "workspace" + workspace.mkdir() + + # Write first version + cycle_data1 = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="rejected", + feedback="First feedback", + skill="test", + elapsed_seconds=5.0, + timestamp="2024-01-15T10:30:00Z", + ) + write_cycle_file(workspace, "TEST-1", "step", cycle_data1) + + # Overwrite with second version + cycle_data2 = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="approved", + feedback="Updated feedback", + skill="test", + elapsed_seconds=8.0, + timestamp="2024-01-15T10:35:00Z", + ) + write_cycle_file(workspace, "TEST-1", "step", cycle_data2) + + output_file = workspace / ".forge" / "reviews" / "TEST-1__step" / "review_cycle_1.json" + data = json.loads(output_file.read_text(encoding="utf-8")) + + # Should have the updated values + assert data["verdict"] == "approved" + assert data["feedback"] == "Updated feedback" + assert data["elapsed_seconds"] == 8.0 + + def test_empty_feedback(self, tmp_path: Path): + """Empty feedback string is preserved.""" + workspace = tmp_path / "workspace" + workspace.mkdir() + + cycle_data = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="approved", + feedback="", + skill="test", + elapsed_seconds=5.0, + timestamp="2024-01-15T10:30:00Z", + ) + + write_cycle_file(workspace, "TEST-1", "step", cycle_data) + + output_file = workspace / ".forge" / "reviews" / "TEST-1__step" / "review_cycle_1.json" + data = json.loads(output_file.read_text(encoding="utf-8")) + + assert data["feedback"] == "" + + def test_multiline_feedback(self, tmp_path: Path): + """Multiline feedback is preserved correctly.""" + workspace = tmp_path / "workspace" + workspace.mkdir() + + multiline_feedback = """Fix the following issues: +1. Missing error handling +2. No unit tests +3. Documentation incomplete""" + + cycle_data = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="rejected", + feedback=multiline_feedback, + skill="code-review", + elapsed_seconds=15.0, + timestamp="2024-01-15T10:30:00Z", + ) + + write_cycle_file(workspace, "TEST-1", "step", cycle_data) + + output_file = workspace / ".forge" / "reviews" / "TEST-1__step" / "review_cycle_1.json" + data = json.loads(output_file.read_text(encoding="utf-8")) + + assert data["feedback"] == multiline_feedback + assert "1. Missing error handling" in data["feedback"] + assert "2. No unit tests" in data["feedback"] + + def test_special_characters_in_feedback(self, tmp_path: Path): + """Special characters in feedback are handled correctly.""" + workspace = tmp_path / "workspace" + workspace.mkdir() + + cycle_data = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="rejected", + feedback='Fix "quotes", , and unicode: émoji 🎉', + skill="test", + elapsed_seconds=5.0, + timestamp="2024-01-15T10:30:00Z", + ) + + write_cycle_file(workspace, "TEST-1", "step", cycle_data) + + output_file = workspace / ".forge" / "reviews" / "TEST-1__step" / "review_cycle_1.json" + data = json.loads(output_file.read_text(encoding="utf-8")) + + assert data["feedback"] == 'Fix "quotes", , and unicode: émoji 🎉' + + def test_elapsed_seconds_float_precision(self, tmp_path: Path): + """Float precision for elapsed_seconds is preserved.""" + workspace = tmp_path / "workspace" + workspace.mkdir() + + cycle_data = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="approved", + feedback="", + skill="test", + elapsed_seconds=123.456789, + timestamp="2024-01-15T10:30:00Z", + ) + + write_cycle_file(workspace, "TEST-1", "step", cycle_data) + + output_file = workspace / ".forge" / "reviews" / "TEST-1__step" / "review_cycle_1.json" + data = json.loads(output_file.read_text(encoding="utf-8")) + + assert data["elapsed_seconds"] == 123.456789 + + def test_step_name_with_hyphens(self, tmp_path: Path): + """Step name with hyphens is handled correctly.""" + workspace = tmp_path / "workspace" + workspace.mkdir() + + cycle_data = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="approved", + feedback="", + skill="test", + elapsed_seconds=5.0, + timestamp="2024-01-15T10:30:00Z", + ) + + write_cycle_file(workspace, "TEST-1", "local-code-review", cycle_data) + + assert ( + workspace / ".forge" / "reviews" / "TEST-1__local-code-review" / "review_cycle_1.json" + ).exists() + + def test_step_name_with_underscores(self, tmp_path: Path): + """Step name with underscores is handled correctly.""" + workspace = tmp_path / "workspace" + workspace.mkdir() + + cycle_data = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="approved", + feedback="", + skill="test", + elapsed_seconds=5.0, + timestamp="2024-01-15T10:30:00Z", + ) + + write_cycle_file(workspace, "TEST-1", "my_step_name", cycle_data) + + assert ( + workspace / ".forge" / "reviews" / "TEST-1__my_step_name" / "review_cycle_1.json" + ).exists() diff --git a/tests/unit/observability/__init__.py b/tests/unit/observability/__init__.py new file mode 100644 index 00000000..da6a2955 --- /dev/null +++ b/tests/unit/observability/__init__.py @@ -0,0 +1 @@ +"""Unit tests for the observability module.""" diff --git a/tests/unit/observability/test_review_poller.py b/tests/unit/observability/test_review_poller.py new file mode 100644 index 00000000..b86900ac --- /dev/null +++ b/tests/unit/observability/test_review_poller.py @@ -0,0 +1,698 @@ +"""Unit tests for the ReviewCyclePoller class.""" + +import asyncio +import json +from pathlib import Path +from unittest.mock import AsyncMock, patch + +import pytest + +from forge.config import Settings +from forge.observability.review_poller import ( + MAX_JSON_PARSE_RETRIES, + ReviewCycleData, + ReviewCyclePoller, +) + +# --------------------------------------------------------------------------- +# ReviewCycleData tests +# --------------------------------------------------------------------------- + + +class TestReviewCycleData: + """Tests for ReviewCycleData dataclass.""" + + def test_all_fields(self): + """Test that all fields are correctly stored.""" + data = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="approved", + feedback="Looks good!", + skill="code-review", + elapsed_seconds=12.5, + timestamp="2024-01-15T10:30:00Z", + file_path="/path/to/file.json", + ) + assert data.cycle == 1 + assert data.max_cycles == 3 + assert data.verdict == "approved" + assert data.feedback == "Looks good!" + assert data.skill == "code-review" + assert data.elapsed_seconds == 12.5 + assert data.timestamp == "2024-01-15T10:30:00Z" + assert data.file_path == "/path/to/file.json" + + def test_default_file_path(self): + """Test that file_path defaults to empty string.""" + data = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="rejected", + feedback="Needs work", + skill="review", + elapsed_seconds=5.0, + timestamp="2024-01-15T10:30:00Z", + ) + assert data.file_path == "" + + def test_from_dict_all_fields(self): + """Test creating from dict with all fields.""" + input_dict = { + "cycle": 2, + "max_cycles": 5, + "verdict": "rejected", + "feedback": "Fix bugs", + "skill": "local-code-review", + "elapsed_seconds": 8.3, + "timestamp": "2024-01-16T14:20:00Z", + } + data = ReviewCycleData.from_dict(input_dict, file_path="/test/path.json") + + assert data.cycle == 2 + assert data.max_cycles == 5 + assert data.verdict == "rejected" + assert data.feedback == "Fix bugs" + assert data.skill == "local-code-review" + assert data.elapsed_seconds == 8.3 + assert data.timestamp == "2024-01-16T14:20:00Z" + assert data.file_path == "/test/path.json" + + def test_from_dict_minimal_fields(self): + """Test creating from dict with only required fields.""" + input_dict = { + "cycle": 1, + "max_cycles": 3, + "verdict": "approved", + } + data = ReviewCycleData.from_dict(input_dict) + + assert data.cycle == 1 + assert data.max_cycles == 3 + assert data.verdict == "approved" + assert data.feedback == "" + assert data.skill == "" + assert data.elapsed_seconds == 0.0 + assert data.timestamp == "" + assert data.file_path == "" + + def test_from_dict_missing_required_raises(self): + """Test that missing required fields raise KeyError.""" + with pytest.raises(KeyError): + ReviewCycleData.from_dict({"cycle": 1, "max_cycles": 3}) # missing verdict + + with pytest.raises(KeyError): + ReviewCycleData.from_dict({"cycle": 1, "verdict": "approved"}) # missing max_cycles + + +# --------------------------------------------------------------------------- +# ReviewCyclePoller initialization tests +# --------------------------------------------------------------------------- + + +class TestReviewCyclePollerInit: + """Tests for ReviewCyclePoller initialization.""" + + def test_init_basic(self): + """Test basic initialization.""" + poller = ReviewCyclePoller( + workspace_path=Path("/workspace"), + step_name="implement_task", + ) + assert poller.workspace_path == Path("/workspace") + assert poller.step_name == "implement_task" + assert poller._settings is None + assert poller._processed_files == set() + assert poller._running is False + + def test_init_with_settings(self, mock_settings): + """Test initialization with explicit settings.""" + poller = ReviewCyclePoller( + workspace_path=Path("/workspace"), + step_name="generate_prd", + settings=mock_settings, + ) + assert poller._settings is mock_settings + + def test_review_cycle_dir(self): + """Test review_cycle_dir property.""" + poller = ReviewCyclePoller( + workspace_path=Path("/workspace"), + step_name="implement_task", + ) + assert poller.review_cycle_dir == Path("/workspace/.forge/implement_task") + + +class TestBuildCycleDir: + """Tests for build_cycle_dir static method.""" + + def test_with_task_key_and_skill_name(self): + """Test with task_key + skill_name returns reviews path.""" + result = ReviewCyclePoller.build_cycle_dir( + Path("/workspace"), "AISOS-2126", "implement-task", "implement_task" + ) + assert result == Path("/workspace/.forge/reviews/AISOS-2126__implement-task") + + def test_without_task_key_falls_back_to_step_name(self): + """Test without task_key falls back to step_name path.""" + result = ReviewCyclePoller.build_cycle_dir(Path("/workspace"), "", "", "implement_task") + assert result == Path("/workspace/.forge/implement_task") + + def test_poll_interval_from_settings(self, mock_settings): + """Test poll_interval reads from settings.""" + mock_settings.auto_review_poll_interval = 10.0 + poller = ReviewCyclePoller( + workspace_path=Path("/workspace"), + step_name="test_step", + settings=mock_settings, + ) + assert poller.poll_interval == 10.0 + + +# --------------------------------------------------------------------------- +# ReviewCyclePoller._get_review_cycle_files tests +# --------------------------------------------------------------------------- + + +class TestGetReviewCycleFiles: + """Tests for _get_review_cycle_files method.""" + + def test_no_directory_returns_empty(self, tmp_path): + """Test that non-existent directory returns empty list.""" + poller = ReviewCyclePoller( + workspace_path=tmp_path, + step_name="nonexistent_step", + ) + assert poller._get_review_cycle_files() == [] + + def test_empty_directory_returns_empty(self, tmp_path): + """Test that empty directory returns empty list.""" + step_dir = tmp_path / ".forge" / "test_step" + step_dir.mkdir(parents=True) + + poller = ReviewCyclePoller( + workspace_path=tmp_path, + step_name="test_step", + ) + assert poller._get_review_cycle_files() == [] + + def test_finds_review_cycle_files(self, tmp_path): + """Test finding review_cycle_*.json files.""" + step_dir = tmp_path / ".forge" / "test_step" + step_dir.mkdir(parents=True) + + # Create matching files + (step_dir / "review_cycle_1.json").write_text("{}") + (step_dir / "review_cycle_2.json").write_text("{}") + (step_dir / "review_cycle_10.json").write_text("{}") + + # Create non-matching files + (step_dir / "other_file.json").write_text("{}") + (step_dir / "review_cycle.json").write_text("{}") # Missing underscore + (step_dir / "review_cycle_1.txt").write_text("{}") # Wrong extension + + poller = ReviewCyclePoller( + workspace_path=tmp_path, + step_name="test_step", + ) + files = poller._get_review_cycle_files() + + assert len(files) == 3 + assert all(f.name.startswith("review_cycle_") for f in files) + assert all(f.suffix == ".json" for f in files) + + def test_files_are_sorted(self, tmp_path): + """Test that files are returned in sorted order.""" + step_dir = tmp_path / ".forge" / "test_step" + step_dir.mkdir(parents=True) + + # Create files in non-sorted order + (step_dir / "review_cycle_3.json").write_text("{}") + (step_dir / "review_cycle_1.json").write_text("{}") + (step_dir / "review_cycle_2.json").write_text("{}") + + poller = ReviewCyclePoller( + workspace_path=tmp_path, + step_name="test_step", + ) + files = poller._get_review_cycle_files() + + assert [f.name for f in files] == [ + "review_cycle_1.json", + "review_cycle_2.json", + "review_cycle_3.json", + ] + + +# --------------------------------------------------------------------------- +# ReviewCyclePoller._parse_json_with_retry tests +# --------------------------------------------------------------------------- + + +class TestParseJsonWithRetry: + """Tests for _parse_json_with_retry method.""" + + @pytest.mark.asyncio + async def test_parse_valid_json(self, tmp_path): + """Test parsing a valid JSON file.""" + json_file = tmp_path / "test.json" + json_file.write_text('{"cycle": 1, "max_cycles": 3, "verdict": "approved"}') + + poller = ReviewCyclePoller( + workspace_path=tmp_path, + step_name="test", + ) + result = await poller._parse_json_with_retry(json_file) + + assert result == {"cycle": 1, "max_cycles": 3, "verdict": "approved"} + + @pytest.mark.asyncio + async def test_parse_empty_file_returns_none(self, tmp_path): + """Test that empty file returns None after retries.""" + json_file = tmp_path / "empty.json" + json_file.write_text("") + + poller = ReviewCyclePoller( + workspace_path=tmp_path, + step_name="test", + ) + + with patch("asyncio.sleep", new_callable=AsyncMock) as mock_sleep: + result = await poller._parse_json_with_retry(json_file) + + assert result is None + # Should have retried MAX_JSON_PARSE_RETRIES - 1 times + assert mock_sleep.await_count == MAX_JSON_PARSE_RETRIES - 1 + + @pytest.mark.asyncio + async def test_parse_invalid_json_retries(self, tmp_path): + """Test that invalid JSON triggers retries.""" + json_file = tmp_path / "invalid.json" + json_file.write_text('{"incomplete": ') + + poller = ReviewCyclePoller( + workspace_path=tmp_path, + step_name="test", + ) + + with patch("asyncio.sleep", new_callable=AsyncMock) as mock_sleep: + result = await poller._parse_json_with_retry(json_file) + + assert result is None + assert mock_sleep.await_count == MAX_JSON_PARSE_RETRIES - 1 + + @pytest.mark.asyncio + async def test_parse_file_not_found_returns_none(self, tmp_path): + """Test that missing file returns None without retries.""" + poller = ReviewCyclePoller( + workspace_path=tmp_path, + step_name="test", + ) + result = await poller._parse_json_with_retry(tmp_path / "nonexistent.json") + + assert result is None + + @pytest.mark.asyncio + async def test_parse_succeeds_on_retry(self, tmp_path): + """Test that parsing can succeed after initial failures.""" + json_file = tmp_path / "eventually_valid.json" + json_file.write_text('{"incomplete": ') + + poller = ReviewCyclePoller( + workspace_path=tmp_path, + step_name="test", + ) + + call_count = 0 + original_read_text = Path.read_text + + def mock_read_text(self_path, encoding="utf-8"): + nonlocal call_count + # Only intercept reads for our test file + if self_path == json_file: + call_count += 1 + if call_count < 3: + return '{"incomplete": ' + return '{"cycle": 1, "max_cycles": 3, "verdict": "approved"}' + return original_read_text(self_path, encoding=encoding) + + with ( + patch.object(Path, "read_text", mock_read_text), + patch("asyncio.sleep", new_callable=AsyncMock), + ): + result = await poller._parse_json_with_retry(json_file) + + assert result == {"cycle": 1, "max_cycles": 3, "verdict": "approved"} + + +# --------------------------------------------------------------------------- +# ReviewCyclePoller.poll_once tests +# --------------------------------------------------------------------------- + + +class TestPollOnce: + """Tests for poll_once method.""" + + @pytest.mark.asyncio + async def test_poll_once_no_files(self, tmp_path): + """Test poll_once with no files.""" + poller = ReviewCyclePoller( + workspace_path=tmp_path, + step_name="test_step", + ) + result = await poller.poll_once() + assert result == [] + + @pytest.mark.asyncio + async def test_poll_once_new_files(self, tmp_path): + """Test poll_once detects new files.""" + step_dir = tmp_path / ".forge" / "test_step" + step_dir.mkdir(parents=True) + + cycle_data = { + "cycle": 1, + "max_cycles": 3, + "verdict": "approved", + "feedback": "LGTM", + "skill": "code-review", + "elapsed_seconds": 5.5, + "timestamp": "2024-01-15T10:00:00Z", + } + (step_dir / "review_cycle_1.json").write_text(json.dumps(cycle_data)) + + poller = ReviewCyclePoller( + workspace_path=tmp_path, + step_name="test_step", + ) + result = await poller.poll_once() + + assert len(result) == 1 + assert result[0].cycle == 1 + assert result[0].verdict == "approved" + assert result[0].feedback == "LGTM" + assert result[0].skill == "code-review" + + @pytest.mark.asyncio + async def test_poll_once_skips_already_processed(self, tmp_path): + """Test that poll_once skips already processed files.""" + step_dir = tmp_path / ".forge" / "test_step" + step_dir.mkdir(parents=True) + + cycle_data = { + "cycle": 1, + "max_cycles": 3, + "verdict": "approved", + "feedback": "", + "skill": "", + "elapsed_seconds": 0, + "timestamp": "", + } + file_path = step_dir / "review_cycle_1.json" + file_path.write_text(json.dumps(cycle_data)) + + poller = ReviewCyclePoller( + workspace_path=tmp_path, + step_name="test_step", + ) + + # First poll should detect the file + result1 = await poller.poll_once() + assert len(result1) == 1 + + # Second poll should skip it + result2 = await poller.poll_once() + assert len(result2) == 0 + + @pytest.mark.asyncio + async def test_poll_once_detects_new_after_first(self, tmp_path): + """Test that poll_once detects new files on subsequent polls.""" + step_dir = tmp_path / ".forge" / "test_step" + step_dir.mkdir(parents=True) + + cycle1 = {"cycle": 1, "max_cycles": 3, "verdict": "rejected", "feedback": "Fix it"} + (step_dir / "review_cycle_1.json").write_text(json.dumps(cycle1)) + + poller = ReviewCyclePoller( + workspace_path=tmp_path, + step_name="test_step", + ) + + result1 = await poller.poll_once() + assert len(result1) == 1 + assert result1[0].verdict == "rejected" + + # Add new file + cycle2 = {"cycle": 2, "max_cycles": 3, "verdict": "approved", "feedback": ""} + (step_dir / "review_cycle_2.json").write_text(json.dumps(cycle2)) + + result2 = await poller.poll_once() + assert len(result2) == 1 + assert result2[0].cycle == 2 + assert result2[0].verdict == "approved" + + @pytest.mark.asyncio + async def test_poll_once_skips_invalid_json(self, tmp_path): + """Test that invalid JSON files are skipped.""" + step_dir = tmp_path / ".forge" / "test_step" + step_dir.mkdir(parents=True) + + # Valid file + valid_data = {"cycle": 1, "max_cycles": 3, "verdict": "approved"} + (step_dir / "review_cycle_1.json").write_text(json.dumps(valid_data)) + + # Invalid JSON file + (step_dir / "review_cycle_2.json").write_text("not valid json") + + poller = ReviewCyclePoller( + workspace_path=tmp_path, + step_name="test_step", + ) + + with patch("asyncio.sleep", new_callable=AsyncMock): + result = await poller.poll_once() + + # Only valid file should be returned + assert len(result) == 1 + assert result[0].cycle == 1 + + @pytest.mark.asyncio + async def test_poll_once_skips_missing_required_fields(self, tmp_path): + """Test that files with missing required fields are skipped.""" + step_dir = tmp_path / ".forge" / "test_step" + step_dir.mkdir(parents=True) + + # Missing 'verdict' + invalid_data = {"cycle": 1, "max_cycles": 3} + (step_dir / "review_cycle_1.json").write_text(json.dumps(invalid_data)) + + poller = ReviewCyclePoller( + workspace_path=tmp_path, + step_name="test_step", + ) + result = await poller.poll_once() + + assert len(result) == 0 + + +# --------------------------------------------------------------------------- +# ReviewCyclePoller async iteration tests +# --------------------------------------------------------------------------- + + +class TestRunLoop: + """Tests for run_loop(callback) interface.""" + + @pytest.mark.asyncio + async def test_run_loop_sets_running(self, tmp_path, mock_settings): + """Test that run_loop() sets _running to True.""" + mock_settings.auto_review_poll_interval = 0.01 + poller = ReviewCyclePoller( + workspace_path=tmp_path, + step_name="test", + settings=mock_settings, + ) + + running_during_loop = False + + def callback(_cycles): + nonlocal running_during_loop + running_during_loop = poller._running + poller.stop() + + # Create a file so the callback fires + step_dir = tmp_path / ".forge" / "test" + step_dir.mkdir(parents=True) + (step_dir / "review_cycle_1.json").write_text( + json.dumps({"cycle": 1, "max_cycles": 3, "verdict": "approved"}) + ) + + await poller.run_loop(callback) + assert running_during_loop is True + + @pytest.mark.asyncio + async def test_stop_ends_run_loop(self, tmp_path, mock_settings): + """Test that stop() ends run_loop.""" + mock_settings.auto_review_poll_interval = 0.01 + poller = ReviewCyclePoller( + workspace_path=tmp_path, + step_name="test", + settings=mock_settings, + ) + + async def stop_soon(): + await asyncio.sleep(0.05) + poller.stop() + + asyncio.get_event_loop().create_task(stop_soon()) + await poller.run_loop(lambda _cycles: None) + + assert poller._running is False + + @pytest.mark.asyncio + async def test_run_loop_respects_poll_interval(self, tmp_path, mock_settings): + """Test that run_loop waits for poll interval between polls.""" + mock_settings.auto_review_poll_interval = 0.5 + poller = ReviewCyclePoller( + workspace_path=tmp_path, + step_name="test", + settings=mock_settings, + ) + + with patch("asyncio.sleep", new_callable=AsyncMock) as mock_sleep: + # Stop after first sleep call + mock_sleep.side_effect = [None, asyncio.CancelledError()] + with pytest.raises(asyncio.CancelledError): + await poller.run_loop(lambda _cycles: None) + + mock_sleep.assert_awaited_with(0.5) + + @pytest.mark.asyncio + async def test_run_loop_calls_callback_with_new_cycles(self, tmp_path, mock_settings): + """Test that run_loop invokes callback with detected cycles.""" + mock_settings.auto_review_poll_interval = 0.01 + step_dir = tmp_path / ".forge" / "test_step" + step_dir.mkdir(parents=True) + + cycle_data = {"cycle": 1, "max_cycles": 3, "verdict": "approved"} + (step_dir / "review_cycle_1.json").write_text(json.dumps(cycle_data)) + + poller = ReviewCyclePoller( + workspace_path=tmp_path, + step_name="test_step", + settings=mock_settings, + ) + + results = [] + call_count = 0 + + def callback(new_cycles): + nonlocal call_count + call_count += 1 + results.extend(new_cycles) + poller.stop() + + await poller.run_loop(callback) + + assert len(results) == 1 + assert results[0].verdict == "approved" + + +# --------------------------------------------------------------------------- +# Integration-style tests +# --------------------------------------------------------------------------- + + +class TestPollerIntegration: + """Integration-style tests for the poller.""" + + @pytest.mark.asyncio + async def test_detects_files_within_time_limit(self, tmp_path, mock_settings): + """Test that new files are detected within acceptable time.""" + mock_settings.auto_review_poll_interval = 0.05 # Fast polling for test + step_dir = tmp_path / ".forge" / "implement_task" + step_dir.mkdir(parents=True) + + poller = ReviewCyclePoller( + workspace_path=tmp_path, + step_name="implement_task", + settings=mock_settings, + ) + + # Create file before starting loop (simulates file appearing during execution) + cycle_data = { + "cycle": 1, + "max_cycles": 3, + "verdict": "rejected", + "feedback": "Tests failing", + "skill": "local-code-review", + "elapsed_seconds": 15.2, + "timestamp": "2024-01-15T12:00:00Z", + } + (step_dir / "review_cycle_1.json").write_text(json.dumps(cycle_data)) + + detected = [] + + def callback(new_cycles): + detected.extend(new_cycles) + poller.stop() + + await poller.run_loop(callback) + + assert len(detected) == 1 + assert detected[0].cycle == 1 + assert detected[0].verdict == "rejected" + assert detected[0].feedback == "Tests failing" + + @pytest.mark.asyncio + async def test_multiple_cycle_files_in_order(self, tmp_path): + """Test processing multiple review cycles in order.""" + step_dir = tmp_path / ".forge" / "implement_task" + step_dir.mkdir(parents=True) + + # Create multiple cycle files + for i in range(1, 4): + verdict = "approved" if i == 3 else "rejected" + data = { + "cycle": i, + "max_cycles": 3, + "verdict": verdict, + "feedback": f"Feedback for cycle {i}" if verdict == "rejected" else "", + "skill": "local-code-review", + "elapsed_seconds": i * 5.0, + "timestamp": f"2024-01-15T10:{i:02d}:00Z", + } + (step_dir / f"review_cycle_{i}.json").write_text(json.dumps(data)) + + poller = ReviewCyclePoller( + workspace_path=tmp_path, + step_name="implement_task", + ) + + result = await poller.poll_once() + + assert len(result) == 3 + assert result[0].cycle == 1 + assert result[0].verdict == "rejected" + assert result[1].cycle == 2 + assert result[1].verdict == "rejected" + assert result[2].cycle == 3 + assert result[2].verdict == "approved" + + +# --------------------------------------------------------------------------- +# Fixture for mock_settings +# --------------------------------------------------------------------------- + + +@pytest.fixture +def mock_settings(): + """Create mock settings for tests.""" + return Settings( + redis_url="redis://localhost:6379/0", + jira_base_url="https://test.atlassian.net", + jira_api_token="test-token", + jira_user_email="test@example.com", + github_token="test-github-token", + anthropic_api_key="test-anthropic-key", + auto_review_poll_interval=5.0, + ) diff --git a/tests/unit/observability/test_review_recorder.py b/tests/unit/observability/test_review_recorder.py new file mode 100644 index 00000000..0f0b7412 --- /dev/null +++ b/tests/unit/observability/test_review_recorder.py @@ -0,0 +1,524 @@ +"""Unit tests for the ReviewCycleRecorder class.""" + +import logging +from pathlib import Path +from unittest.mock import patch + +import pytest + +from forge.observability.review_poller import ReviewCycleData +from forge.observability.review_recorder import ReviewCycleRecorder + +# --------------------------------------------------------------------------- +# ReviewCycleRecorder initialization tests +# --------------------------------------------------------------------------- + + +class TestReviewCycleRecorderInit: + """Tests for ReviewCycleRecorder initialization.""" + + def test_init_with_step_name_only(self): + """Test initialization with just step_name.""" + recorder = ReviewCycleRecorder(step_name="implement_task") + + assert recorder.step_name == "implement_task" + assert recorder.mode is None + assert recorder.recording_dir is None + + def test_init_with_log_mode(self): + """Test initialization with log mode.""" + recorder = ReviewCycleRecorder( + step_name="generate_prd", + mode="log", + ) + + assert recorder.step_name == "generate_prd" + assert recorder.mode == "log" + + def test_init_with_copy_mode_and_recording_dir(self): + """Test initialization with copy mode and recording directory.""" + recorder = ReviewCycleRecorder( + step_name="implement_task", + mode="copy", + recording_dir=Path("/recordings"), + ) + + assert recorder.step_name == "implement_task" + assert recorder.mode == "copy" + assert recorder.recording_dir == Path("/recordings") + + def test_init_copy_mode_requires_recording_dir(self): + """Test that copy mode raises ValueError without recording_dir.""" + with pytest.raises(ValueError, match="recording_dir is required"): + ReviewCycleRecorder( + step_name="test_step", + mode="copy", + ) + + def test_init_log_mode_does_not_require_recording_dir(self): + """Test that log mode works without recording_dir.""" + recorder = ReviewCycleRecorder( + step_name="test_step", + mode="log", + ) + assert recorder.recording_dir is None + + def test_step_dir_property(self): + """Test step_dir property returns correct path.""" + recorder = ReviewCycleRecorder( + step_name="implement_task", + mode="copy", + recording_dir=Path("/recordings"), + ) + + assert recorder.step_dir == Path("/recordings/implement_task") + + def test_step_dir_none_when_no_recording_dir(self): + """Test step_dir returns None when no recording_dir.""" + recorder = ReviewCycleRecorder( + step_name="test_step", + mode="log", + ) + assert recorder.step_dir is None + + +# --------------------------------------------------------------------------- +# ReviewCycleRecorder.record tests (log mode) +# --------------------------------------------------------------------------- + + +class TestReviewCycleRecorderRecord: + """Tests for ReviewCycleRecorder.record method.""" + + def test_record_disabled_mode_does_nothing(self, caplog): + """Test that record does nothing when mode is None.""" + recorder = ReviewCycleRecorder(step_name="test_step", mode=None) + ts = "2024-01-15T10:30:00+00:00" + data = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="approved", + feedback="Good", + skill="review", + elapsed_seconds=1.0, + timestamp=ts, + ) + + with caplog.at_level(logging.INFO): + recorder.record(data) + + # No log messages should be recorded + assert len(caplog.records) == 0 + + def test_record_log_mode_logs_at_info_level(self, caplog): + """Test that log mode records at INFO level.""" + recorder = ReviewCycleRecorder(step_name="implement_task", mode="log") + ts = "2024-01-15T10:30:00+00:00" + data = ReviewCycleData( + cycle=2, + max_cycles=5, + verdict="rejected", + feedback="Fix the tests", + skill="code-review", + elapsed_seconds=12.5, + timestamp=ts, + ) + + with caplog.at_level(logging.INFO): + recorder.record(data) + + assert len(caplog.records) == 1 + assert caplog.records[0].levelno == logging.INFO + + def test_record_log_mode_includes_cycle_info(self, caplog): + """Test that log output includes cycle number and max cycles.""" + recorder = ReviewCycleRecorder(step_name="test_step", mode="log") + ts = "2024-01-15T10:30:00+00:00" + data = ReviewCycleData( + cycle=3, + max_cycles=5, + verdict="approved", + feedback="", + skill="review", + elapsed_seconds=1.0, + timestamp=ts, + ) + + with caplog.at_level(logging.INFO): + recorder.record(data) + + log_message = caplog.records[0].message + assert "3/5" in log_message or ("3" in log_message and "5" in log_message) + + def test_record_log_mode_includes_step_name(self, caplog): + """Test that log output includes step name.""" + recorder = ReviewCycleRecorder(step_name="implement_task", mode="log") + ts = "2024-01-15T10:30:00+00:00" + data = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="approved", + feedback="", + skill="review", + elapsed_seconds=1.0, + timestamp=ts, + ) + + with caplog.at_level(logging.INFO): + recorder.record(data) + + log_message = caplog.records[0].message + assert "implement_task" in log_message + + def test_record_log_mode_includes_verdict(self, caplog): + """Test that log output includes verdict.""" + recorder = ReviewCycleRecorder(step_name="test_step", mode="log") + ts = "2024-01-15T10:30:00+00:00" + data = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="rejected", + feedback="", + skill="review", + elapsed_seconds=1.0, + timestamp=ts, + ) + + with caplog.at_level(logging.INFO): + recorder.record(data) + + log_message = caplog.records[0].message + assert "rejected" in log_message + + def test_record_log_mode_includes_skill(self, caplog): + """Test that log output includes skill name.""" + recorder = ReviewCycleRecorder(step_name="test_step", mode="log") + ts = "2024-01-15T10:30:00+00:00" + data = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="approved", + feedback="", + skill="local-code-review", + elapsed_seconds=1.0, + timestamp=ts, + ) + + with caplog.at_level(logging.INFO): + recorder.record(data) + + log_message = caplog.records[0].message + assert "local-code-review" in log_message + + def test_record_log_mode_includes_elapsed_seconds(self, caplog): + """Test that log output includes elapsed seconds.""" + recorder = ReviewCycleRecorder(step_name="test_step", mode="log") + ts = "2024-01-15T10:30:00+00:00" + data = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="approved", + feedback="", + skill="review", + elapsed_seconds=15.75, + timestamp=ts, + ) + + with caplog.at_level(logging.INFO): + recorder.record(data) + + log_message = caplog.records[0].message + assert "15.75" in log_message + + def test_record_log_mode_includes_feedback(self, caplog): + """Test that log output includes feedback.""" + recorder = ReviewCycleRecorder(step_name="test_step", mode="log") + ts = "2024-01-15T10:30:00+00:00" + data = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="rejected", + feedback="Fix the bug in line 42", + skill="review", + elapsed_seconds=1.0, + timestamp=ts, + ) + + with caplog.at_level(logging.INFO): + recorder.record(data) + + log_message = caplog.records[0].message + assert "Fix the bug in line 42" in log_message + + def test_record_log_mode_truncates_long_feedback(self, caplog): + """Test that log output truncates feedback longer than 100 chars.""" + recorder = ReviewCycleRecorder(step_name="test_step", mode="log") + ts = "2024-01-15T10:30:00+00:00" + long_feedback = "x" * 150 + data = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="rejected", + feedback=long_feedback, + skill="review", + elapsed_seconds=1.0, + timestamp=ts, + ) + + with caplog.at_level(logging.INFO): + recorder.record(data) + + log_message = caplog.records[0].message + # Should truncate and add ellipsis + assert "..." in log_message + # Should not contain the full feedback + assert long_feedback not in log_message + + +# --------------------------------------------------------------------------- +# ReviewCycleRecorder.record_file tests (copy mode) +# --------------------------------------------------------------------------- + + +class TestReviewCycleRecorderRecordFile: + """Tests for ReviewCycleRecorder.record_file method.""" + + def test_record_file_disabled_mode_returns_none(self, tmp_path): + """Test that record_file returns None when mode is None.""" + recorder = ReviewCycleRecorder(step_name="test_step", mode=None) + + # Create a source file + source_file = tmp_path / "review_cycle_1.json" + source_file.write_text('{"cycle": 1}') + + result = recorder.record_file(source_file) + assert result is None + + def test_record_file_log_mode_returns_none(self, tmp_path): + """Test that record_file returns None when mode is log.""" + recorder = ReviewCycleRecorder(step_name="test_step", mode="log") + + # Create a source file + source_file = tmp_path / "review_cycle_1.json" + source_file.write_text('{"cycle": 1}') + + result = recorder.record_file(source_file) + assert result is None + + def test_record_file_copy_mode_creates_step_directory(self, tmp_path): + """Test that copy mode creates the step-specific subdirectory.""" + recording_dir = tmp_path / "recordings" + recorder = ReviewCycleRecorder( + step_name="implement_task", + mode="copy", + recording_dir=recording_dir, + ) + + # Create a source file + source_dir = tmp_path / "source" + source_dir.mkdir() + source_file = source_dir / "review_cycle_1.json" + source_file.write_text('{"cycle": 1}') + + recorder.record_file(source_file) + + step_dir = recording_dir / "implement_task" + assert step_dir.exists() + assert step_dir.is_dir() + + def test_record_file_copy_mode_copies_file(self, tmp_path): + """Test that copy mode copies the file to recording directory.""" + recording_dir = tmp_path / "recordings" + recorder = ReviewCycleRecorder( + step_name="implement_task", + mode="copy", + recording_dir=recording_dir, + ) + + # Create a source file with content + source_dir = tmp_path / "source" + source_dir.mkdir() + source_file = source_dir / "review_cycle_1.json" + source_file.write_text('{"cycle": 1, "verdict": "approved"}') + + result = recorder.record_file(source_file) + + expected_dest = recording_dir / "implement_task" / "review_cycle_1.json" + assert result == expected_dest + assert expected_dest.exists() + assert expected_dest.read_text() == '{"cycle": 1, "verdict": "approved"}' + + def test_record_file_copy_mode_preserves_filename(self, tmp_path): + """Test that copy mode preserves the original filename.""" + recording_dir = tmp_path / "recordings" + recorder = ReviewCycleRecorder( + step_name="test_step", + mode="copy", + recording_dir=recording_dir, + ) + + source_dir = tmp_path / "source" + source_dir.mkdir() + source_file = source_dir / "review_cycle_5.json" + source_file.write_text('{"cycle": 5}') + + result = recorder.record_file(source_file) + + assert result is not None + assert result.name == "review_cycle_5.json" + + def test_record_file_copy_mode_handles_multiple_files(self, tmp_path): + """Test that multiple files can be copied.""" + recording_dir = tmp_path / "recordings" + recorder = ReviewCycleRecorder( + step_name="test_step", + mode="copy", + recording_dir=recording_dir, + ) + + source_dir = tmp_path / "source" + source_dir.mkdir() + + # Copy multiple files + for i in range(1, 4): + source_file = source_dir / f"review_cycle_{i}.json" + source_file.write_text(f'{{"cycle": {i}}}') + recorder.record_file(source_file) + + step_dir = recording_dir / "test_step" + assert (step_dir / "review_cycle_1.json").exists() + assert (step_dir / "review_cycle_2.json").exists() + assert (step_dir / "review_cycle_3.json").exists() + + def test_record_file_nonexistent_source_returns_none(self, tmp_path, caplog): + """Test that nonexistent source file returns None and logs warning.""" + recording_dir = tmp_path / "recordings" + recorder = ReviewCycleRecorder( + step_name="test_step", + mode="copy", + recording_dir=recording_dir, + ) + + nonexistent_file = tmp_path / "missing.json" + + with caplog.at_level(logging.WARNING): + result = recorder.record_file(nonexistent_file) + + assert result is None + assert any("does not exist" in record.message for record in caplog.records) + + def test_record_file_returns_path_on_success(self, tmp_path): + """Test that successful copy returns the destination path.""" + recording_dir = tmp_path / "recordings" + recorder = ReviewCycleRecorder( + step_name="my_step", + mode="copy", + recording_dir=recording_dir, + ) + + source_dir = tmp_path / "source" + source_dir.mkdir() + source_file = source_dir / "review_cycle_1.json" + source_file.write_text('{"cycle": 1}') + + result = recorder.record_file(source_file) + + assert result is not None + assert result == recording_dir / "my_step" / "review_cycle_1.json" + + def test_record_file_directory_creation_error_returns_none(self, tmp_path, caplog): + """Test that directory creation error returns None.""" + # Create a file where the directory should be + recording_dir = tmp_path / "recordings" + recording_dir.mkdir() + blocking_file = recording_dir / "test_step" + blocking_file.write_text("blocking file") + + recorder = ReviewCycleRecorder( + step_name="test_step", + mode="copy", + recording_dir=recording_dir, + ) + + source_dir = tmp_path / "source" + source_dir.mkdir() + source_file = source_dir / "review_cycle_1.json" + source_file.write_text('{"cycle": 1}') + + with caplog.at_level(logging.ERROR): + result = recorder.record_file(source_file) + + assert result is None + assert any("Failed to create directory" in record.message for record in caplog.records) + + def test_record_file_copy2_oserror_returns_none(self, tmp_path, caplog): + """Test that shutil.copy2 failure returns None and logs error.""" + recording_dir = tmp_path / "recordings" + recorder = ReviewCycleRecorder( + step_name="test_step", + mode="copy", + recording_dir=recording_dir, + ) + + source_dir = tmp_path / "source" + source_dir.mkdir() + source_file = source_dir / "review_cycle_1.json" + source_file.write_text('{"cycle": 1}') + + with ( + patch("shutil.copy2", side_effect=OSError("Permission denied")), + caplog.at_level(logging.ERROR), + ): + result = recorder.record_file(source_file) + + assert result is None + assert any("Failed to copy file" in record.message for record in caplog.records) + + +# --------------------------------------------------------------------------- +# Settings configuration tests +# --------------------------------------------------------------------------- + + +class TestSettingsConfiguration: + """Tests for AUTO_REVIEW_RECORD_POLLED_FILES configuration.""" + + def test_setting_default_is_none(self): + """Test that the default value is None.""" + from forge.config import Settings + + with patch.dict("os.environ", {}, clear=True): + # Create settings with minimal required fields + settings = Settings( + jira_base_url="https://example.atlassian.net", + jira_api_token="token", + jira_user_email="user@example.com", + github_token="ghtoken", + ) + assert settings.auto_review_record_polled_files is None + + def test_setting_accepts_log_value(self): + """Test that 'log' value is accepted.""" + from forge.config import Settings + + with patch.dict("os.environ", {"AUTO_REVIEW_RECORD_POLLED_FILES": "log"}, clear=False): + settings = Settings( + jira_base_url="https://example.atlassian.net", + jira_api_token="token", + jira_user_email="user@example.com", + github_token="ghtoken", + ) + assert settings.auto_review_record_polled_files == "log" + + def test_setting_accepts_copy_value(self): + """Test that 'copy' value is accepted.""" + from forge.config import Settings + + with patch.dict("os.environ", {"AUTO_REVIEW_RECORD_POLLED_FILES": "copy"}, clear=False): + settings = Settings( + jira_base_url="https://example.atlassian.net", + jira_api_token="token", + jira_user_email="user@example.com", + github_token="ghtoken", + ) + assert settings.auto_review_record_polled_files == "copy" diff --git a/tests/unit/sandbox/test_runner_review_polling.py b/tests/unit/sandbox/test_runner_review_polling.py new file mode 100644 index 00000000..a10ddaba --- /dev/null +++ b/tests/unit/sandbox/test_runner_review_polling.py @@ -0,0 +1,1184 @@ +"""Unit tests for review polling integration in ContainerRunner.""" + +import asyncio +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from forge.observability import ReviewCycleData +from forge.observability.review_poller import ReviewCyclePoller +from forge.sandbox.runner import ( + ContainerResult, + ContainerRunner, +) + +# --------------------------------------------------------------------------- +# Helper to create a runner instance without __init__ side effects +# --------------------------------------------------------------------------- + + +def _runner_without_init() -> ContainerRunner: + """Create a ContainerRunner instance without running __init__.""" + return object.__new__(ContainerRunner) + + +# --------------------------------------------------------------------------- +# ContainerResult tests +# --------------------------------------------------------------------------- + + +class TestContainerResultReviewCycles: + """Tests for ContainerResult review_cycles field.""" + + def test_default_review_cycles_empty(self): + """Test that review_cycles defaults to empty list.""" + result = ContainerResult( + success=True, + exit_code=0, + stdout="", + stderr="", + ) + assert result.review_cycles == [] + + def test_review_cycles_with_data(self): + """Test that review_cycles can store ReviewCycleData.""" + cycles = [ + ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="rejected", + feedback="Fix the bug", + skill="local-code-review", + elapsed_seconds=5.5, + timestamp="2024-01-15T10:30:00Z", + ), + ReviewCycleData( + cycle=2, + max_cycles=3, + verdict="approved", + feedback="", + skill="local-code-review", + elapsed_seconds=3.2, + timestamp="2024-01-15T10:35:00Z", + ), + ] + result = ContainerResult( + success=True, + exit_code=0, + stdout="output", + stderr="", + review_cycles=cycles, + ) + assert len(result.review_cycles) == 2 + assert result.review_cycles[0].verdict == "rejected" + assert result.review_cycles[1].verdict == "approved" + + +# --------------------------------------------------------------------------- +# ContainerRunner.run() with step_name tests +# --------------------------------------------------------------------------- + + +class TestRunWithStepName: + """Tests for ContainerRunner.run() with step_name parameter.""" + + @pytest.mark.asyncio + async def test_run_accepts_step_name_parameter(self, tmp_path: Path): + """Test that run() accepts the step_name parameter.""" + runner = _runner_without_init() + runner.settings = MagicMock() + runner.settings.container_image = "test:latest" + runner.settings.container_timeout = 60 + runner.settings.container_memory = "1g" + runner.settings.container_cpus = "1" + runner.settings.container_keep = False + runner.settings.auto_review_poll_interval = 1.0 + runner.settings.auto_review_record_polled_files = None + + # Create a mock process that completes immediately + mock_process = MagicMock() + mock_process.communicate = AsyncMock(return_value=(b"output", b"")) + mock_process.returncode = 0 + + with ( + patch.object(runner, "_build_container_name", return_value="test-container"), + patch.object(runner, "_build_podman_command", return_value=["podman", "run"]), + patch( + "forge.sandbox.runner.asyncio.create_subprocess_exec", + return_value=mock_process, + ), + ): + result = await runner.run( + workspace_path=tmp_path, + task_summary="Test task", + task_description="Test description", + step_name="implement_task", + ) + + assert result.success is True + assert result.review_cycles == [] + + @pytest.mark.asyncio + async def test_run_without_step_name_disables_polling(self, tmp_path: Path): + """Test that run() without step_name disables polling.""" + runner = _runner_without_init() + runner.settings = MagicMock() + runner.settings.container_image = "test:latest" + runner.settings.container_timeout = 60 + runner.settings.container_memory = "1g" + runner.settings.container_cpus = "1" + runner.settings.container_keep = False + + mock_process = MagicMock() + mock_process.communicate = AsyncMock(return_value=(b"output", b"")) + mock_process.returncode = 0 + + with ( + patch.object(runner, "_build_container_name", return_value="test-container"), + patch.object(runner, "_build_podman_command", return_value=["podman", "run"]), + patch( + "forge.sandbox.runner.asyncio.create_subprocess_exec", + return_value=mock_process, + ), + patch("forge.sandbox.runner.ReviewCyclePoller") as mock_poller_class, + ): + result = await runner.run( + workspace_path=tmp_path, + task_summary="Test task", + task_description="Test description", + # No step_name provided + ) + + # Poller should not be created when step_name is None + mock_poller_class.assert_not_called() + assert result.review_cycles == [] + + +# --------------------------------------------------------------------------- +# Background polling task tests +# --------------------------------------------------------------------------- + + +class TestBackgroundPollingTask: + """Tests for the background polling task during container execution.""" + + @pytest.mark.asyncio + async def test_polling_task_started_with_step_name(self, tmp_path: Path): + """Test that polling task is started when step_name is provided.""" + runner = _runner_without_init() + runner.settings = MagicMock() + runner.settings.container_image = "test:latest" + runner.settings.container_timeout = 60 + runner.settings.container_memory = "1g" + runner.settings.container_cpus = "1" + runner.settings.container_keep = False + runner.settings.auto_review_poll_interval = 1.0 + runner.settings.auto_review_record_polled_files = None + + mock_process = MagicMock() + mock_process.communicate = AsyncMock(return_value=(b"output", b"")) + mock_process.returncode = 0 + + # Track if polling was started + poller_created = False + + def create_poller(*_args, **_kwargs): + nonlocal poller_created + poller_created = True + mock_poller = MagicMock() + mock_poller.run_loop = AsyncMock() + mock_poller.poll_once = AsyncMock(return_value=[]) + mock_poller.stop = MagicMock() + mock_poller.step_name = "implement_task" + return mock_poller + + with ( + patch.object(runner, "_build_container_name", return_value="test-container"), + patch.object(runner, "_build_podman_command", return_value=["podman", "run"]), + patch( + "forge.sandbox.runner.asyncio.create_subprocess_exec", + return_value=mock_process, + ), + patch( + "forge.sandbox.runner.ReviewCyclePoller", + side_effect=create_poller, + ), + patch("forge.sandbox.runner.ReviewCycleRecorder"), + ): + await runner.run( + workspace_path=tmp_path, + task_summary="Test task", + task_description="Test description", + step_name="implement_task", + ) + + assert poller_created is True + + @pytest.mark.asyncio + async def test_polling_task_cancelled_on_container_exit(self, tmp_path: Path): + """Test that polling task is cancelled when container exits.""" + runner = _runner_without_init() + runner.settings = MagicMock() + runner.settings.container_image = "test:latest" + runner.settings.container_timeout = 60 + runner.settings.container_memory = "1g" + runner.settings.container_cpus = "1" + runner.settings.container_keep = False + runner.settings.auto_review_poll_interval = 1.0 + runner.settings.auto_review_record_polled_files = None + + mock_process = MagicMock() + mock_process.communicate = AsyncMock(return_value=(b"output", b"")) + mock_process.returncode = 0 + + stop_called = False + + def create_poller(*_args, **_kwargs): + mock_poller = MagicMock() + mock_poller.run_loop = AsyncMock() + mock_poller.poll_once = AsyncMock(return_value=[]) + + def stop(): + nonlocal stop_called + stop_called = True + + mock_poller.stop = stop + mock_poller.step_name = "implement_task" + return mock_poller + + with ( + patch.object(runner, "_build_container_name", return_value="test-container"), + patch.object(runner, "_build_podman_command", return_value=["podman", "run"]), + patch( + "forge.sandbox.runner.asyncio.create_subprocess_exec", + return_value=mock_process, + ), + patch( + "forge.sandbox.runner.ReviewCyclePoller", + side_effect=create_poller, + ), + patch("forge.sandbox.runner.ReviewCycleRecorder"), + ): + await runner.run( + workspace_path=tmp_path, + task_summary="Test task", + task_description="Test description", + step_name="implement_task", + ) + + assert stop_called is True + + +# --------------------------------------------------------------------------- +# Review cycle collection tests +# --------------------------------------------------------------------------- + + +class TestReviewCycleCollection: + """Tests for collecting review cycles into ContainerResult.""" + + @pytest.mark.asyncio + async def test_detected_cycles_added_to_result(self, tmp_path: Path): + """Test that detected review cycles are added to the result.""" + runner = _runner_without_init() + runner.settings = MagicMock() + runner.settings.container_image = "test:latest" + runner.settings.container_timeout = 60 + runner.settings.container_memory = "1g" + runner.settings.container_cpus = "1" + runner.settings.container_keep = False + runner.settings.auto_review_poll_interval = 0.1 + runner.settings.auto_review_record_polled_files = "log" + + mock_process = MagicMock() + mock_process.communicate = AsyncMock(return_value=(b"output", b"")) + mock_process.returncode = 0 + + # Simulate a review cycle file being detected during final poll + detected_cycle = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="approved", + feedback="LGTM", + skill="local-code-review", + elapsed_seconds=5.0, + timestamp="2024-01-15T10:30:00Z", + ) + + def create_poller(*_args, **_kwargs): + mock_poller = MagicMock() + mock_poller.run_loop = AsyncMock() + mock_poller.poll_once = AsyncMock(return_value=[detected_cycle]) + mock_poller.stop = MagicMock() + mock_poller.step_name = "implement_task" + return mock_poller + + with ( + patch.object(runner, "_build_container_name", return_value="test-container"), + patch.object(runner, "_build_podman_command", return_value=["podman", "run"]), + patch( + "forge.sandbox.runner.asyncio.create_subprocess_exec", + return_value=mock_process, + ), + patch( + "forge.sandbox.runner.ReviewCyclePoller", + side_effect=create_poller, + ), + patch("forge.sandbox.runner.ReviewCycleRecorder"), + patch("forge.sandbox.runner.record_review_cycle"), + patch("forge.sandbox.runner.record_review_verdict"), + patch("forge.sandbox.runner.observe_review_duration"), + ): + result = await runner.run( + workspace_path=tmp_path, + task_summary="Test task", + task_description="Test description", + step_name="implement_task", + ) + + assert len(result.review_cycles) == 1 + assert result.review_cycles[0].verdict == "approved" + assert result.review_cycles[0].skill == "local-code-review" + + @pytest.mark.asyncio + async def test_cycles_collected_even_on_timeout(self, tmp_path: Path): + """Test that cycles are collected even when container times out.""" + runner = _runner_without_init() + runner.settings = MagicMock() + runner.settings.container_image = "test:latest" + runner.settings.container_timeout = 1 + runner.settings.container_memory = "1g" + runner.settings.container_cpus = "1" + runner.settings.container_keep = False + runner.settings.auto_review_poll_interval = 0.1 + runner.settings.auto_review_record_polled_files = None + + mock_process = MagicMock() + mock_process.communicate = AsyncMock(side_effect=TimeoutError()) + mock_process.returncode = None + + # Pre-collected cycle (simulating one detected before timeout) + timeout_cycle = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="rejected", + feedback="Partial review", + skill="local-code-review", + elapsed_seconds=2.0, + timestamp="2024-01-15T10:30:00Z", + ) + + cycles_collected = [] + + def create_poller(*_args, **_kwargs): + mock_poller = MagicMock() + + async def run_loop(callback): + # Deliver one cycle via callback, then block (simulating ongoing polling) + cycles_collected.append(timeout_cycle) + callback([timeout_cycle]) + await asyncio.sleep(1000) + + mock_poller.run_loop = run_loop + mock_poller.poll_once = AsyncMock(return_value=[]) + mock_poller.stop = MagicMock() + mock_poller.step_name = "implement_task" + return mock_poller + + with ( + patch.object(runner, "_build_container_name", return_value="test-container"), + patch.object(runner, "_build_podman_command", return_value=["podman", "run"]), + patch( + "forge.sandbox.runner.asyncio.create_subprocess_exec", + return_value=mock_process, + ), + patch.object(runner, "_stop_timed_out_container", new=AsyncMock()), + patch( + "forge.sandbox.runner.ReviewCyclePoller", + side_effect=create_poller, + ), + patch("forge.sandbox.runner.ReviewCycleRecorder"), + patch("forge.sandbox.runner.record_review_cycle"), + patch("forge.sandbox.runner.record_review_verdict"), + patch("forge.sandbox.runner.observe_review_duration"), + ): + result = await runner.run( + workspace_path=tmp_path, + task_summary="Test task", + task_description="Test description", + step_name="implement_task", + ) + + # Result should indicate failure + assert result.success is False + assert "Timeout" in (result.error_message or "") + assert isinstance(result.review_cycles, list) + + +# --------------------------------------------------------------------------- +# Metrics recording tests +# --------------------------------------------------------------------------- + + +class TestMetricsRecording: + """Tests for Prometheus metrics recording during polling.""" + + @pytest.mark.asyncio + async def test_metrics_recorded_for_detected_cycles(self, tmp_path: Path): + """Test that metrics are recorded for each detected cycle.""" + runner = _runner_without_init() + runner.settings = MagicMock() + runner.settings.container_image = "test:latest" + runner.settings.container_timeout = 60 + runner.settings.container_memory = "1g" + runner.settings.container_cpus = "1" + runner.settings.container_keep = False + runner.settings.auto_review_poll_interval = 0.1 + runner.settings.auto_review_record_polled_files = None + + mock_process = MagicMock() + mock_process.communicate = AsyncMock(return_value=(b"output", b"")) + mock_process.returncode = 0 + + detected_cycle = ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="approved", + feedback="", + skill="implement-task", + elapsed_seconds=10.5, + timestamp="2024-01-15T10:30:00Z", + ) + + def create_poller(*_args, **_kwargs): + mock_poller = MagicMock() + mock_poller.run_loop = AsyncMock() + mock_poller.poll_once = AsyncMock(return_value=[detected_cycle]) + mock_poller.stop = MagicMock() + mock_poller.step_name = "implement_task" + return mock_poller + + with ( + patch.object(runner, "_build_container_name", return_value="test-container"), + patch.object(runner, "_build_podman_command", return_value=["podman", "run"]), + patch( + "forge.sandbox.runner.asyncio.create_subprocess_exec", + return_value=mock_process, + ), + patch( + "forge.sandbox.runner.ReviewCyclePoller", + side_effect=create_poller, + ), + patch("forge.sandbox.runner.ReviewCycleRecorder"), + patch("forge.sandbox.runner.record_review_cycle") as mock_cycle, + patch("forge.sandbox.runner.record_review_verdict") as mock_verdict, + patch("forge.sandbox.runner.observe_review_duration") as mock_duration, + ): + await runner.run( + workspace_path=tmp_path, + task_summary="Test task", + task_description="Test description", + step_name="implement_task", + ) + + # Verify metrics were recorded + mock_cycle.assert_called_with("implement-task", "implement_task") + mock_verdict.assert_called_with("implement-task", "implement_task", "approved") + mock_duration.assert_called_with("implement-task", "implement_task", 10.5) + + +# --------------------------------------------------------------------------- +# Step name path organization tests +# --------------------------------------------------------------------------- + + +class TestStepNamePathOrganization: + """Tests for step-name based path organization.""" + + @pytest.mark.asyncio + async def test_step_name_passed_to_poller(self, tmp_path: Path): + """Test that step_name is passed to the poller correctly.""" + runner = _runner_without_init() + runner.settings = MagicMock() + runner.settings.container_image = "test:latest" + runner.settings.container_timeout = 60 + runner.settings.container_memory = "1g" + runner.settings.container_cpus = "1" + runner.settings.container_keep = False + runner.settings.auto_review_poll_interval = 1.0 + runner.settings.auto_review_record_polled_files = None + + mock_process = MagicMock() + mock_process.communicate = AsyncMock(return_value=(b"output", b"")) + mock_process.returncode = 0 + + captured_step_name = None + + def create_poller( + workspace_path=None, step_name=None, task_key=None, skill_name=None, settings=None + ): + nonlocal captured_step_name + captured_step_name = step_name + mock_poller = MagicMock() + mock_poller.run_loop = AsyncMock() + mock_poller.poll_once = AsyncMock(return_value=[]) + mock_poller.stop = MagicMock() + mock_poller.step_name = step_name + _ = workspace_path, task_key, skill_name, settings + return mock_poller + + with ( + patch.object(runner, "_build_container_name", return_value="test-container"), + patch.object(runner, "_build_podman_command", return_value=["podman", "run"]), + patch( + "forge.sandbox.runner.asyncio.create_subprocess_exec", + return_value=mock_process, + ), + patch( + "forge.sandbox.runner.ReviewCyclePoller", + side_effect=create_poller, + ), + patch("forge.sandbox.runner.ReviewCycleRecorder"), + ): + await runner.run( + workspace_path=tmp_path, + task_summary="Test task", + task_description="Test description", + step_name="local_review", + ) + + assert captured_step_name == "local_review" + + @pytest.mark.asyncio + async def test_step_name_passed_to_recorder(self, tmp_path: Path): + """Test that step_name is passed to the recorder correctly.""" + runner = _runner_without_init() + runner.settings = MagicMock() + runner.settings.container_image = "test:latest" + runner.settings.container_timeout = 60 + runner.settings.container_memory = "1g" + runner.settings.container_cpus = "1" + runner.settings.container_keep = False + runner.settings.auto_review_poll_interval = 1.0 + runner.settings.auto_review_record_polled_files = "log" + + mock_process = MagicMock() + mock_process.communicate = AsyncMock(return_value=(b"output", b"")) + mock_process.returncode = 0 + + captured_recorder_step_name = None + + def create_poller(**kwargs): + mock_poller = MagicMock() + mock_poller.run_loop = AsyncMock() + mock_poller.poll_once = AsyncMock(return_value=[]) + mock_poller.stop = MagicMock() + mock_poller.step_name = kwargs.get("step_name", "") + return mock_poller + + def create_recorder(step_name=None, mode=None, recording_dir=None): + nonlocal captured_recorder_step_name + captured_recorder_step_name = step_name + mock_recorder = MagicMock() + mock_recorder.record = MagicMock() + mock_recorder.record_file = MagicMock() + # Suppress unused warnings + _ = mode, recording_dir + return mock_recorder + + with ( + patch.object(runner, "_build_container_name", return_value="test-container"), + patch.object(runner, "_build_podman_command", return_value=["podman", "run"]), + patch( + "forge.sandbox.runner.asyncio.create_subprocess_exec", + return_value=mock_process, + ), + patch( + "forge.sandbox.runner.ReviewCyclePoller", + side_effect=create_poller, + ), + patch( + "forge.sandbox.runner.ReviewCycleRecorder", + side_effect=create_recorder, + ), + ): + await runner.run( + workspace_path=tmp_path, + task_summary="Test task", + task_description="Test description", + step_name="fix_ci", + ) + + assert captured_recorder_step_name == "fix_ci" + + +# --------------------------------------------------------------------------- +# _sweep_review_cycles() tests +# --------------------------------------------------------------------------- + + +class TestSweepReviewCycles: + """Tests for the _sweep_review_cycles() post-execution sweep.""" + + def test_sweep_finds_missed_file(self, tmp_path: Path, caplog): + """Test that sweep catches files missed during async polling.""" + import json + import logging + + runner = _runner_without_init() + + # Create a review cycle file that was NOT processed by the poller + step_name = "implement_task" + cycle_dir = tmp_path / ".forge" / step_name + cycle_dir.mkdir(parents=True) + + cycle_data = { + "cycle": 1, + "max_cycles": 3, + "verdict": "approved", + "feedback": "Looks good", + "skill": "local-review", + "elapsed_seconds": 5.5, + "timestamp": "2024-01-15T10:30:00Z", + } + cycle_file = cycle_dir / "review_cycle_1.json" + cycle_file.write_text(json.dumps(cycle_data)) + + # Empty processed files set - nothing was caught during polling + processed_files: set[str] = set() + collected_cycles: list[ReviewCycleData] = [] + + # Mock recorder + mock_recorder = MagicMock() + + with caplog.at_level(logging.WARNING): + runner._sweep_review_cycles( + workspace_path=tmp_path, + step_name=step_name, + processed_files=processed_files, + collected_cycles=collected_cycles, + recorder=mock_recorder, + ) + + # Should have found the missed file + assert len(collected_cycles) == 1 + assert collected_cycles[0].cycle == 1 + assert collected_cycles[0].verdict == "approved" + assert collected_cycles[0].skill == "local-review" + + # Should log a warning about missed files + assert "Sweep caught 1 review cycle file(s) missed" in caplog.text + assert step_name in caplog.text + + def test_sweep_deduplicates_against_processed_files(self, tmp_path: Path): + """Test that sweep skips files already processed by async poller.""" + import json + + runner = _runner_without_init() + + step_name = "implement_task" + cycle_dir = tmp_path / ".forge" / step_name + cycle_dir.mkdir(parents=True) + + # Create two cycle files + for i in [1, 2]: + cycle_data = { + "cycle": i, + "max_cycles": 3, + "verdict": "approved", + "feedback": f"Review {i}", + "skill": "local-review", + "elapsed_seconds": float(i), + "timestamp": f"2024-01-15T10:3{i}:00Z", + } + cycle_file = cycle_dir / f"review_cycle_{i}.json" + cycle_file.write_text(json.dumps(cycle_data)) + + # Simulate that cycle_1 was already processed + cycle_1_path = str(cycle_dir / "review_cycle_1.json") + processed_files: set[str] = {cycle_1_path} + collected_cycles: list[ReviewCycleData] = [] + + mock_recorder = MagicMock() + + runner._sweep_review_cycles( + workspace_path=tmp_path, + step_name=step_name, + processed_files=processed_files, + collected_cycles=collected_cycles, + recorder=mock_recorder, + ) + + # Should only find cycle_2 (cycle_1 was already processed) + assert len(collected_cycles) == 1 + assert collected_cycles[0].cycle == 2 + + def test_sweep_no_warning_when_no_missed_files(self, tmp_path: Path, caplog): + """Test that no warning is logged when all files were already processed.""" + import json + import logging + + runner = _runner_without_init() + + step_name = "implement_task" + cycle_dir = tmp_path / ".forge" / step_name + cycle_dir.mkdir(parents=True) + + cycle_data = { + "cycle": 1, + "max_cycles": 3, + "verdict": "approved", + "feedback": "", + "skill": "local-review", + "elapsed_seconds": 5.0, + "timestamp": "2024-01-15T10:30:00Z", + } + cycle_file = cycle_dir / "review_cycle_1.json" + cycle_file.write_text(json.dumps(cycle_data)) + + # File was already processed + processed_files: set[str] = {str(cycle_file)} + collected_cycles: list[ReviewCycleData] = [] + + mock_recorder = MagicMock() + + with caplog.at_level(logging.WARNING): + runner._sweep_review_cycles( + workspace_path=tmp_path, + step_name=step_name, + processed_files=processed_files, + collected_cycles=collected_cycles, + recorder=mock_recorder, + ) + + # Should not find any new files + assert len(collected_cycles) == 0 + + # Should not log warning about missed files + assert "Sweep caught" not in caplog.text + + def test_sweep_handles_nonexistent_directory(self, tmp_path: Path): + """Test that sweep handles missing .forge/{step} directory gracefully.""" + runner = _runner_without_init() + + # Don't create the directory + processed_files: set[str] = set() + collected_cycles: list[ReviewCycleData] = [] + + mock_recorder = MagicMock() + + # Should not raise + runner._sweep_review_cycles( + workspace_path=tmp_path, + step_name="nonexistent_step", + processed_files=processed_files, + collected_cycles=collected_cycles, + recorder=mock_recorder, + ) + + assert len(collected_cycles) == 0 + + def test_sweep_handles_invalid_json(self, tmp_path: Path, caplog): + """Test that sweep handles invalid JSON files gracefully.""" + import logging + + runner = _runner_without_init() + + step_name = "implement_task" + cycle_dir = tmp_path / ".forge" / step_name + cycle_dir.mkdir(parents=True) + + # Create an invalid JSON file + cycle_file = cycle_dir / "review_cycle_1.json" + cycle_file.write_text("not valid json {") + + processed_files: set[str] = set() + collected_cycles: list[ReviewCycleData] = [] + + mock_recorder = MagicMock() + + with caplog.at_level(logging.WARNING): + runner._sweep_review_cycles( + workspace_path=tmp_path, + step_name=step_name, + processed_files=processed_files, + collected_cycles=collected_cycles, + recorder=mock_recorder, + ) + + # Should not have collected any cycles + assert len(collected_cycles) == 0 + + # Should log warning about parse failure + assert "Failed to parse review cycle file" in caplog.text + + def test_sweep_handles_missing_required_fields(self, tmp_path: Path, caplog): + """Test that sweep handles JSON with missing required fields.""" + import json + import logging + + runner = _runner_without_init() + + step_name = "implement_task" + cycle_dir = tmp_path / ".forge" / step_name + cycle_dir.mkdir(parents=True) + + # Create JSON missing required fields + cycle_data = {"verdict": "approved", "feedback": "Missing fields"} + cycle_file = cycle_dir / "review_cycle_1.json" + cycle_file.write_text(json.dumps(cycle_data)) + + processed_files: set[str] = set() + collected_cycles: list[ReviewCycleData] = [] + + mock_recorder = MagicMock() + + with caplog.at_level(logging.WARNING): + runner._sweep_review_cycles( + workspace_path=tmp_path, + step_name=step_name, + processed_files=processed_files, + collected_cycles=collected_cycles, + recorder=mock_recorder, + ) + + # Should not have collected any cycles + assert len(collected_cycles) == 0 + + # Should log warning about invalid data + assert "Invalid review cycle data" in caplog.text + + def test_sweep_handles_empty_file(self, tmp_path: Path, caplog): + """Test that sweep handles empty files gracefully.""" + import logging + + runner = _runner_without_init() + + step_name = "implement_task" + cycle_dir = tmp_path / ".forge" / step_name + cycle_dir.mkdir(parents=True) + + # Create an empty file + cycle_file = cycle_dir / "review_cycle_1.json" + cycle_file.write_text("") + + processed_files: set[str] = set() + collected_cycles: list[ReviewCycleData] = [] + + mock_recorder = MagicMock() + + with caplog.at_level(logging.WARNING): + runner._sweep_review_cycles( + workspace_path=tmp_path, + step_name=step_name, + processed_files=processed_files, + collected_cycles=collected_cycles, + recorder=mock_recorder, + ) + + # Should not have collected any cycles + assert len(collected_cycles) == 0 + + # Should log warning about empty file + assert "Empty review cycle file" in caplog.text + + def test_sweep_emits_metrics(self, tmp_path: Path): + """Test that sweep emits Prometheus metrics for caught files.""" + import json + + runner = _runner_without_init() + + step_name = "implement_task" + cycle_dir = tmp_path / ".forge" / step_name + cycle_dir.mkdir(parents=True) + + cycle_data = { + "cycle": 1, + "max_cycles": 3, + "verdict": "rejected", + "feedback": "Needs work", + "skill": "local-review", + "elapsed_seconds": 8.5, + "timestamp": "2024-01-15T10:30:00Z", + } + cycle_file = cycle_dir / "review_cycle_1.json" + cycle_file.write_text(json.dumps(cycle_data)) + + processed_files: set[str] = set() + collected_cycles: list[ReviewCycleData] = [] + + mock_recorder = MagicMock() + + with ( + patch("forge.sandbox.runner.record_review_cycle") as mock_cycle, + patch("forge.sandbox.runner.record_review_verdict") as mock_verdict, + patch("forge.sandbox.runner.observe_review_duration") as mock_duration, + ): + runner._sweep_review_cycles( + workspace_path=tmp_path, + step_name=step_name, + processed_files=processed_files, + collected_cycles=collected_cycles, + recorder=mock_recorder, + ) + + # Verify metrics were emitted + mock_cycle.assert_called_once_with("local-review", step_name) + mock_verdict.assert_called_once_with("local-review", step_name, "rejected") + mock_duration.assert_called_once_with("local-review", step_name, 8.5) + + def test_sweep_records_via_recorder(self, tmp_path: Path): + """Test that sweep uses recorder to record and copy files.""" + import json + + runner = _runner_without_init() + + step_name = "implement_task" + cycle_dir = tmp_path / ".forge" / step_name + cycle_dir.mkdir(parents=True) + + cycle_data = { + "cycle": 1, + "max_cycles": 3, + "verdict": "approved", + "feedback": "", + "skill": "local-review", + "elapsed_seconds": 5.0, + "timestamp": "2024-01-15T10:30:00Z", + } + cycle_file = cycle_dir / "review_cycle_1.json" + cycle_file.write_text(json.dumps(cycle_data)) + + processed_files: set[str] = set() + collected_cycles: list[ReviewCycleData] = [] + + mock_recorder = MagicMock() + + runner._sweep_review_cycles( + workspace_path=tmp_path, + step_name=step_name, + processed_files=processed_files, + collected_cycles=collected_cycles, + recorder=mock_recorder, + ) + + # Verify recorder methods were called + mock_recorder.record.assert_called_once() + mock_recorder.record_file.assert_called_once_with(cycle_file) + + +class TestSweepIntegrationWithRun: + """Tests for sweep integration with ContainerRunner.run().""" + + @pytest.mark.asyncio + async def test_fast_exit_files_caught_by_sweep(self, tmp_path: Path, caplog): + """Test that files written just before container exit are caught by sweep.""" + import json + import logging + + runner = _runner_without_init() + runner.settings = MagicMock() + runner.settings.container_image = "test:latest" + runner.settings.container_timeout = 60 + runner.settings.container_memory = "1g" + runner.settings.container_cpus = "1" + runner.settings.container_keep = False + runner.settings.auto_review_poll_interval = 1.0 + runner.settings.auto_review_record_polled_files = "log" + + mock_process = MagicMock() + mock_process.communicate = AsyncMock(return_value=(b"output", b"")) + mock_process.returncode = 0 + + step_name = "implement_task" + + # Create a file that simulates being written just before container exit + # (not caught by async polling) + cycle_dir = tmp_path / ".forge" / step_name + cycle_dir.mkdir(parents=True) + cycle_data = { + "cycle": 1, + "max_cycles": 3, + "verdict": "approved", + "feedback": "Fast exit", + "skill": "fast-review", + "elapsed_seconds": 1.0, + "timestamp": "2024-01-15T10:30:00Z", + } + cycle_file = cycle_dir / "review_cycle_1.json" + cycle_file.write_text(json.dumps(cycle_data)) + + mock_poller_class = MagicMock() + mock_poller_class.build_cycle_dir = ReviewCyclePoller.build_cycle_dir + + def create_poller(**kwargs): + mock_poller = MagicMock() + # run_loop does nothing (simulating fast exit where files are + # written after polling stops) + mock_poller.run_loop = AsyncMock() + mock_poller.poll_once = AsyncMock(return_value=[]) + mock_poller.stop = MagicMock() + mock_poller.step_name = kwargs.get("step_name", "") + # Empty processed files - nothing was caught during async polling + mock_poller._processed_files = set() + return mock_poller + + mock_poller_class.side_effect = create_poller + + with ( + patch.object(runner, "_build_container_name", return_value="test-container"), + patch.object(runner, "_build_podman_command", return_value=["podman", "run"]), + patch( + "forge.sandbox.runner.asyncio.create_subprocess_exec", + return_value=mock_process, + ), + patch( + "forge.sandbox.runner.ReviewCyclePoller", + mock_poller_class, + ), + patch("forge.sandbox.runner.ReviewCycleRecorder") as mock_recorder_class, + patch("forge.sandbox.runner.record_review_cycle"), + patch("forge.sandbox.runner.record_review_verdict"), + patch("forge.sandbox.runner.observe_review_duration"), + caplog.at_level(logging.WARNING), + ): + mock_recorder = MagicMock() + mock_recorder_class.return_value = mock_recorder + + result = await runner.run( + workspace_path=tmp_path, + task_summary="Test task", + task_description="Test description", + step_name=step_name, + ) + + # The sweep should have caught the file + assert len(result.review_cycles) == 1 + assert result.review_cycles[0].verdict == "approved" + assert result.review_cycles[0].skill == "fast-review" + + # Should log warning about missed files + assert "Sweep caught 1 review cycle file(s) missed" in caplog.text + + +# --------------------------------------------------------------------------- +# _build_container_result tests +# --------------------------------------------------------------------------- + + +class TestBuildContainerResult: + """Tests for ContainerRunner._build_container_result method.""" + + def test_exit_success_returns_success_true(self): + """Test EXIT_SUCCESS returns success=True.""" + runner = _runner_without_init() + runner.settings = MagicMock() + runner.settings.container_keep = False + + result = runner._build_container_result( + exit_code=0, + stdout_str="output", + stderr_str="", + collected_cycles=[], + container_name="test-container", + ) + + assert result.success is True + assert result.exit_code == 0 + assert result.tests_passed is True + assert result.error_message is None + + def test_exit_tests_failed_returns_tests_passed_false(self): + """Test EXIT_TESTS_FAILED returns tests_passed=False.""" + runner = _runner_without_init() + runner.settings = MagicMock() + runner.settings.container_keep = False + + result = runner._build_container_result( + exit_code=2, # EXIT_TESTS_FAILED + stdout_str="output", + stderr_str="test failures", + collected_cycles=[], + container_name="test-container", + ) + + assert result.success is False + assert result.exit_code == 2 + assert result.tests_passed is False + assert result.error_message == "Tests failed after max retries" + + def test_other_exit_code_returns_generic_error(self): + """Test other exit code returns generic error message.""" + runner = _runner_without_init() + runner.settings = MagicMock() + runner.settings.container_keep = False + + result = runner._build_container_result( + exit_code=1, # EXIT_TASK_FAILED + stdout_str="output", + stderr_str="error details", + collected_cycles=[], + container_name="test-container", + ) + + assert result.success is False + assert result.exit_code == 1 + assert result.error_message == "Task failed with exit code 1" + + @pytest.mark.asyncio + async def test_sweep_runs_after_async_polling(self, tmp_path: Path): + """Test that sweep is called after container exits.""" + runner = _runner_without_init() + runner.settings = MagicMock() + runner.settings.container_image = "test:latest" + runner.settings.container_timeout = 60 + runner.settings.container_memory = "1g" + runner.settings.container_cpus = "1" + runner.settings.container_keep = False + runner.settings.auto_review_poll_interval = 1.0 + runner.settings.auto_review_record_polled_files = None + + mock_process = MagicMock() + mock_process.communicate = AsyncMock(return_value=(b"output", b"")) + mock_process.returncode = 0 + + sweep_called = False + original_sweep = runner._sweep_review_cycles + + def mock_sweep(*args, **kwargs): + nonlocal sweep_called + sweep_called = True + return original_sweep(*args, **kwargs) + + def create_poller(**kwargs): + mock_poller = MagicMock() + mock_poller.run_loop = AsyncMock() + mock_poller.poll_once = AsyncMock(return_value=[]) + mock_poller.stop = MagicMock() + mock_poller.step_name = kwargs.get("step_name", "") + mock_poller._processed_files = set() + return mock_poller + + with ( + patch.object(runner, "_build_container_name", return_value="test-container"), + patch.object(runner, "_build_podman_command", return_value=["podman", "run"]), + patch( + "forge.sandbox.runner.asyncio.create_subprocess_exec", + return_value=mock_process, + ), + patch( + "forge.sandbox.runner.ReviewCyclePoller", + side_effect=create_poller, + ), + patch("forge.sandbox.runner.ReviewCycleRecorder"), + patch.object(runner, "_sweep_review_cycles", side_effect=mock_sweep), + ): + await runner.run( + workspace_path=tmp_path, + task_summary="Test task", + task_description="Test description", + step_name="implement_task", + ) + + assert sweep_called is True diff --git a/tests/unit/workflow/nodes/test_implementation.py b/tests/unit/workflow/nodes/test_implementation.py index 90de8ba8..ca34ee5e 100644 --- a/tests/unit/workflow/nodes/test_implementation.py +++ b/tests/unit/workflow/nodes/test_implementation.py @@ -370,3 +370,33 @@ async def test_bug_container_failure_keeps_bug_implementation_node(self): assert result["current_node"] == "implement_bug_fix" assert result["last_error"] == "container failed" assert result["retry_count"] == 1 + + +class TestImplementTaskStepName: + """Tests for step_name propagation to ContainerRunner.""" + + @pytest.mark.asyncio + async def test_passes_step_name_implement_task_to_runner(self): + """ContainerRunner.run() is called with step_name='implement_task'.""" + from forge.workflow.nodes.implementation import implement_task + + mock_jira = _make_mock_jira() + runner = _make_successful_runner() + + with ( + patch( + "forge.workflow.nodes.implementation.JiraClient", + return_value=mock_jira, + ), + patch( + "forge.workflow.nodes.implementation.ContainerRunner", + return_value=runner, + ), + patch("forge.workflow.nodes.implementation.get_settings"), + ): + await implement_task(_make_state()) + + # Verify step_name was passed + runner.run.assert_called_once() + call_kwargs = runner.run.call_args.kwargs + assert call_kwargs.get("step_name") == "implement_task" diff --git a/tests/unit/workflow/nodes/test_local_reviewer.py b/tests/unit/workflow/nodes/test_local_reviewer.py index a343c622..1f5f8149 100644 --- a/tests/unit/workflow/nodes/test_local_reviewer.py +++ b/tests/unit/workflow/nodes/test_local_reviewer.py @@ -5,6 +5,7 @@ import pytest from forge.models.workflow import TicketType +from forge.observability.review_poller import ReviewCycleData from forge.workflow.nodes.local_reviewer import ( _parse_bug_verdict, local_review_changes, @@ -65,6 +66,8 @@ async def run(self, workspace_path, **_kwargs): # noqa: ARG002 result.exit_code = 0 result.stdout = stdout result.stderr = "" + result.review_cycles = [] + result.review_exhausted = False return result return _FakeRunner() @@ -139,6 +142,8 @@ async def run(self, workspace_path, task_description="", **_kwargs): # noqa: AR result.exit_code = 0 result.stdout = "verdict: adequate\n\nfeedback: Good." result.stderr = "" + result.review_cycles = [] + result.review_exhausted = False return result mock_git = _make_mock_git() @@ -352,6 +357,8 @@ async def run(self, workspace_path, task_description="", **_kwargs): # noqa: AR result.exit_code = 0 result.stdout = "No issues found." result.stderr = "" + result.review_cycles = [] + result.review_exhausted = False return result mock_git = _make_mock_git() @@ -386,3 +393,177 @@ async def test_feature_no_qualitative_fields_set(self, base_feature_review_state "qualitative_retry_count" not in result or result.get("qualitative_retry_count") is None ) assert "local_review_verdict" not in result or result.get("local_review_verdict") is None + + +class TestLocalReviewStepName: + """Tests for step_name propagation to ContainerRunner.""" + + @pytest.mark.asyncio + async def test_bug_review_passes_step_name_local_review(self, base_bug_review_state): + """Bug qualitative review passes step_name='local_review' to ContainerRunner.""" + captured_kwargs = [] + + class _CapturingRunner: + async def run(self, **kwargs): + captured_kwargs.append(kwargs) + result = MagicMock() + result.success = True + result.exit_code = 0 + result.stdout = "verdict: adequate\n\nfeedback: Good." + result.stderr = "" + result.review_cycles = [] + result.review_exhausted = False + return result + + mock_git = _make_mock_git() + + with ( + patch( + "forge.workflow.nodes.local_reviewer.ContainerRunner", + return_value=_CapturingRunner(), + ), + patch("forge.workflow.nodes.local_reviewer.GitOperations", return_value=mock_git), + ): + await local_review_changes(base_bug_review_state) + + assert captured_kwargs, "runner.run was not called" + assert captured_kwargs[0].get("step_name") == "local_review" + + @pytest.mark.asyncio + async def test_feature_review_passes_step_name_local_review(self, base_feature_review_state): + """Feature review passes step_name='local_review' to ContainerRunner.""" + captured_kwargs = [] + + class _CapturingRunner: + async def run(self, **kwargs): + captured_kwargs.append(kwargs) + result = MagicMock() + result.success = True + result.exit_code = 0 + result.stdout = "No issues found." + result.stderr = "" + result.review_cycles = [] + result.review_exhausted = False + return result + + mock_git = _make_mock_git() + + with ( + patch( + "forge.workflow.nodes.local_reviewer.ContainerRunner", + return_value=_CapturingRunner(), + ), + patch("forge.workflow.nodes.local_reviewer.GitOperations", return_value=mock_git), + ): + await local_review_changes(base_feature_review_state) + + assert captured_kwargs, "runner.run was not called" + assert captured_kwargs[0].get("step_name") == "local_review" + + +class TestMergeReviewExhaustionIntegration: + """Tests that review_exhaustion_report is populated when review cycles are exhausted.""" + + @pytest.mark.asyncio + async def test_bug_review_populates_exhaustion_report(self, base_bug_review_state): + """Bug review with exhausted cycles populates review_exhaustion_report in state.""" + exhausted_cycles = [ + ReviewCycleData( + cycle=1, + max_cycles=2, + verdict="rejected", + feedback="Tests missing for edge case.", + skill="local-review-bug", + elapsed_seconds=12.5, + timestamp="2026-07-16T10:00:00Z", + ), + ReviewCycleData( + cycle=2, + max_cycles=2, + verdict="rejected", + feedback="Still missing edge case coverage.", + skill="local-review-bug", + elapsed_seconds=14.0, + timestamp="2026-07-16T10:01:00Z", + ), + ] + + class _ExhaustedRunner: + async def run(self, **_kwargs): + result = MagicMock() + result.success = True + result.exit_code = 0 + result.stdout = "verdict: adequate\n\nfeedback: Good." + result.stderr = "" + result.review_cycles = exhausted_cycles + result.review_exhausted = True + return result + + mock_git = _make_mock_git() + + with ( + patch( + "forge.workflow.nodes.local_reviewer.ContainerRunner", + return_value=_ExhaustedRunner(), + ), + patch("forge.workflow.nodes.local_reviewer.GitOperations", return_value=mock_git), + ): + result = await local_review_changes(base_bug_review_state) + + report = result.get("review_exhaustion_report", {}) + assert "BUG-42__local_review" in report + entry = report["BUG-42__local_review"] + assert entry["task_key"] == "BUG-42" + assert entry["step_name"] == "local_review" + + @pytest.mark.asyncio + async def test_feature_review_populates_exhaustion_report(self, base_feature_review_state): + """Feature review with exhausted cycles populates review_exhaustion_report in state.""" + exhausted_cycles = [ + ReviewCycleData( + cycle=1, + max_cycles=2, + verdict="rejected", + feedback="Breaking issue not resolved.", + skill="local-code-review", + elapsed_seconds=10.0, + timestamp="2026-07-16T11:00:00Z", + ), + ReviewCycleData( + cycle=2, + max_cycles=2, + verdict="rejected", + feedback="Breaking issue persists after retry.", + skill="local-code-review", + elapsed_seconds=11.0, + timestamp="2026-07-16T11:01:00Z", + ), + ] + + class _ExhaustedRunner: + async def run(self, **_kwargs): + result = MagicMock() + result.success = True + result.exit_code = 0 + result.stdout = "No issues found." + result.stderr = "" + result.review_cycles = exhausted_cycles + result.review_exhausted = True + return result + + mock_git = _make_mock_git() + + with ( + patch( + "forge.workflow.nodes.local_reviewer.ContainerRunner", + return_value=_ExhaustedRunner(), + ), + patch("forge.workflow.nodes.local_reviewer.GitOperations", return_value=mock_git), + ): + result = await local_review_changes(base_feature_review_state) + + report = result.get("review_exhaustion_report", {}) + assert "FEAT-10__local_review" in report + entry = report["FEAT-10__local_review"] + assert entry["task_key"] == "FEAT-10" + assert entry["step_name"] == "local_review" diff --git a/tests/unit/workflow/nodes/test_task_takeover_review.py b/tests/unit/workflow/nodes/test_task_takeover_review.py index a6193ebb..1f64a9f4 100644 --- a/tests/unit/workflow/nodes/test_task_takeover_review.py +++ b/tests/unit/workflow/nodes/test_task_takeover_review.py @@ -53,6 +53,8 @@ def _make_mock_runner(stdout: str) -> MagicMock: result.stdout = stdout result.stderr = "" result.error_message = None + result.review_cycles = [] + result.review_exhausted = False runner.run = AsyncMock(return_value=result) return runner @@ -107,7 +109,10 @@ async def test_run_qualitative_review_success(self, base_task_state: TaskTakeove with ( patch("forge.workflow.nodes.task_takeover_review.JiraClient", return_value=mock_jira), patch("forge.workflow.nodes.task_takeover_review.GitOperations") as mock_git, - patch("forge.workflow.nodes.task_takeover_review.ContainerRunner", return_value=mock_runner), + patch( + "forge.workflow.nodes.task_takeover_review.ContainerRunner", + return_value=mock_runner, + ), patch("forge.workflow.nodes.task_takeover_review.prepare_workspace") as mock_prepare, ): mock_git_instance = MagicMock() @@ -148,7 +153,10 @@ async def test_run_qualitative_review_tests_incomplete( with ( patch("forge.workflow.nodes.task_takeover_review.JiraClient", return_value=mock_jira), patch("forge.workflow.nodes.task_takeover_review.GitOperations") as mock_git, - patch("forge.workflow.nodes.task_takeover_review.ContainerRunner", return_value=mock_runner), + patch( + "forge.workflow.nodes.task_takeover_review.ContainerRunner", + return_value=mock_runner, + ), patch("forge.workflow.nodes.task_takeover_review.prepare_workspace") as mock_prepare, ): mock_git_instance = MagicMock() @@ -191,11 +199,12 @@ async def test_run_qualitative_review_exception_handling( mock_jira = _make_mock_jira() mock_jira.get_issue = AsyncMock(side_effect=RuntimeError("Jira connection failure")) - with patch( - "forge.workflow.nodes.task_takeover_review.JiraClient", return_value=mock_jira - ), patch( - "forge.workflow.nodes.task_takeover_review.prepare_workspace", - return_value=("/tmp/fake-workspace-review", MagicMock()), + with ( + patch("forge.workflow.nodes.task_takeover_review.JiraClient", return_value=mock_jira), + patch( + "forge.workflow.nodes.task_takeover_review.prepare_workspace", + return_value=("/tmp/fake-workspace-review", MagicMock()), + ), ): result = await run_qualitative_review(base_task_state) diff --git a/tests/unit/workflow/utils/test_review_report.py b/tests/unit/workflow/utils/test_review_report.py new file mode 100644 index 00000000..03026d0d --- /dev/null +++ b/tests/unit/workflow/utils/test_review_report.py @@ -0,0 +1,339 @@ +"""Unit tests for review exhaustion reporting utility.""" + +from forge.observability.review_poller import ReviewCycleData +from forge.sandbox.runner import ContainerResult +from forge.workflow.nodes.pr_creation import _format_review_exhaustion_section +from forge.workflow.utils.review_report import collect_review_exhaustion, merge_review_exhaustion + + +class TestReviewExhausted: + """Tests for ContainerResult.review_exhausted property.""" + + def test_no_cycles_not_exhausted(self): + result = ContainerResult(success=True, exit_code=0, stdout="", stderr="") + assert result.review_exhausted is False + + def test_approved_not_exhausted(self): + result = ContainerResult( + success=True, + exit_code=0, + stdout="", + stderr="", + review_cycles=[ + ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="approved", + feedback="", + skill="test", + elapsed_seconds=1.0, + timestamp="", + ), + ], + ) + assert result.review_exhausted is False + + def test_rejected_below_max_not_exhausted(self): + result = ContainerResult( + success=True, + exit_code=0, + stdout="", + stderr="", + review_cycles=[ + ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="rejected", + feedback="fix it", + skill="test", + elapsed_seconds=1.0, + timestamp="", + ), + ], + ) + assert result.review_exhausted is False + + def test_rejected_at_max_is_exhausted(self): + result = ContainerResult( + success=True, + exit_code=0, + stdout="", + stderr="", + review_cycles=[ + ReviewCycleData( + cycle=1, + max_cycles=2, + verdict="rejected", + feedback="fix it", + skill="test", + elapsed_seconds=1.0, + timestamp="", + ), + ReviewCycleData( + cycle=2, + max_cycles=2, + verdict="rejected", + feedback="still broken", + skill="test", + elapsed_seconds=1.0, + timestamp="", + ), + ], + ) + assert result.review_exhausted is True + + def test_approved_at_max_not_exhausted(self): + result = ContainerResult( + success=True, + exit_code=0, + stdout="", + stderr="", + review_cycles=[ + ReviewCycleData( + cycle=1, + max_cycles=2, + verdict="rejected", + feedback="fix it", + skill="test", + elapsed_seconds=1.0, + timestamp="", + ), + ReviewCycleData( + cycle=2, + max_cycles=2, + verdict="approved", + feedback="", + skill="test", + elapsed_seconds=1.0, + timestamp="", + ), + ], + ) + assert result.review_exhausted is False + + +class TestCollectReviewExhaustion: + """Tests for collect_review_exhaustion utility.""" + + def test_returns_none_when_not_exhausted(self): + result = ContainerResult(success=True, exit_code=0, stdout="", stderr="") + assert collect_review_exhaustion(result, "TASK-1", "implement_task") is None + + def test_returns_none_when_approved(self): + result = ContainerResult( + success=True, + exit_code=0, + stdout="", + stderr="", + review_cycles=[ + ReviewCycleData( + cycle=1, + max_cycles=3, + verdict="approved", + feedback="", + skill="implement-task", + elapsed_seconds=5.0, + timestamp="", + ), + ], + ) + assert collect_review_exhaustion(result, "TASK-1", "implement_task") is None + + def test_returns_dict_when_exhausted(self): + result = ContainerResult( + success=True, + exit_code=0, + stdout="", + stderr="", + review_cycles=[ + ReviewCycleData( + cycle=1, + max_cycles=2, + verdict="rejected", + feedback="missing tests", + skill="implement-task", + elapsed_seconds=5.0, + timestamp="", + ), + ReviewCycleData( + cycle=2, + max_cycles=2, + verdict="rejected", + feedback="still missing tests", + skill="implement-task", + elapsed_seconds=3.0, + timestamp="", + ), + ], + ) + result_tuple = collect_review_exhaustion(result, "AISOS-2053", "implement_task") + + assert result_tuple is not None + key, entry = result_tuple + assert key == "AISOS-2053__implement_task" + assert entry["task_key"] == "AISOS-2053" + assert entry["step_name"] == "implement_task" + assert entry["skill"] == "implement-task" + assert entry["max_retries"] == 2 + assert entry["final_feedback"] == "still missing tests" + assert len(entry["cycles"]) == 2 + assert entry["cycles"][0]["verdict"] == "rejected" + assert entry["cycles"][1]["feedback"] == "still missing tests" + + +class TestFormatReviewExhaustionSection: + """Tests for _format_review_exhaustion_section.""" + + def test_empty_report_returns_empty(self): + assert _format_review_exhaustion_section({}) == "" + + def test_single_entry(self): + report = { + "AISOS-2053__implement_task": { + "task_key": "AISOS-2053", + "step_name": "implement_task", + "skill": "implement-task", + "max_retries": 3, + "final_feedback": "Missing test coverage", + "cycles": [ + {"cycle": 1, "verdict": "rejected", "feedback": "No tests"}, + {"cycle": 2, "verdict": "rejected", "feedback": "Still no tests"}, + {"cycle": 3, "verdict": "rejected", "feedback": "Missing test coverage"}, + ], + }, + } + section = _format_review_exhaustion_section(report) + + assert "Auto-Review Notes" in section + assert "implement_task — AISOS-2053" in section + assert "implement-task" in section + assert "3/3 exhausted" in section + assert "Missing test coverage" in section + + def test_multiple_entries(self): + report = { + "AISOS-2053__implement_task": { + "task_key": "AISOS-2053", + "step_name": "implement_task", + "skill": "implement-task", + "max_retries": 2, + "final_feedback": "No docstrings", + "cycles": [], + }, + "AISOS-2055__implement_task": { + "task_key": "AISOS-2055", + "step_name": "implement_task", + "skill": "implement-task", + "max_retries": 2, + "final_feedback": "Bare except", + "cycles": [], + }, + } + section = _format_review_exhaustion_section(report) + + assert "AISOS-2053" in section + assert "AISOS-2055" in section + assert "No docstrings" in section + assert "Bare except" in section + + def test_multiline_feedback(self): + report = { + "TASK-1__local_review": { + "task_key": "TASK-1", + "step_name": "local_review", + "skill": "local-code-review", + "max_retries": 1, + "final_feedback": "Line 1\nLine 2\nLine 3", + "cycles": [], + }, + } + section = _format_review_exhaustion_section(report) + + assert "> Line 1" in section + assert "> Line 2" in section + assert "> Line 3" in section + + +class TestMergeReviewExhaustion: + """Tests for merge_review_exhaustion utility.""" + + def test_returns_state_unchanged_when_not_exhausted(self): + """Test that state is returned unchanged when review is not exhausted.""" + state = {"some_key": "some_value", "other": 42} + result = ContainerResult(success=True, exit_code=0, stdout="", stderr="") + + merged = merge_review_exhaustion(state, result, "TASK-1", "implement_task") + + assert merged is state + assert "review_exhaustion_report" not in merged + + def test_returns_state_with_report_when_exhausted(self): + """Test that state includes review_exhaustion_report when exhausted.""" + state = {"some_key": "some_value"} + result = ContainerResult( + success=True, + exit_code=0, + stdout="", + stderr="", + review_cycles=[ + ReviewCycleData( + cycle=1, + max_cycles=2, + verdict="rejected", + feedback="missing tests", + skill="implement-task", + elapsed_seconds=5.0, + timestamp="", + ), + ReviewCycleData( + cycle=2, + max_cycles=2, + verdict="rejected", + feedback="still missing tests", + skill="implement-task", + elapsed_seconds=3.0, + timestamp="", + ), + ], + ) + + merged = merge_review_exhaustion(state, result, "AISOS-2053", "implement_task") + + assert "review_exhaustion_report" in merged + report = merged["review_exhaustion_report"] + assert "AISOS-2053__implement_task" in report + assert report["AISOS-2053__implement_task"]["task_key"] == "AISOS-2053" + assert report["AISOS-2053__implement_task"]["step_name"] == "implement_task" + # Original state keys preserved + assert merged["some_key"] == "some_value" + + def test_dict_merge_preserves_existing_state_entries(self): + """Test that the merge creates a new dict containing original state entries.""" + state = {"existing_key": "existing_value", "count": 10} + result = ContainerResult( + success=True, + exit_code=0, + stdout="", + stderr="", + review_cycles=[ + ReviewCycleData( + cycle=1, + max_cycles=1, + verdict="rejected", + feedback="bad code", + skill="impl", + elapsed_seconds=1.0, + timestamp="", + ), + ], + ) + + merged = merge_review_exhaustion(state, result, "T-1", "step") + + # Original entries are preserved + assert merged["existing_key"] == "existing_value" + assert merged["count"] == 10 + # New report entry is present + assert "review_exhaustion_report" in merged + # Original state is not mutated + assert "review_exhaustion_report" not in state diff --git a/uv.lock b/uv.lock index 8bd6b6d6..b3750e74 100644 --- a/uv.lock +++ b/uv.lock @@ -1,5 +1,5 @@ version = 1 -revision = 3 +revision = 2 requires-python = ">=3.11" resolution-markers = [ "python_full_version >= '3.14'",