diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 80435c56..391dc394 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -23,6 +23,7 @@ Thank you for your interest in contributing to Auto Claude! This document provid - [Pull Request Targets](#pull-request-targets) - [Release Process](#release-process-maintainers) - [Commit Messages](#commit-messages) + - [PR Hygiene](#pr-hygiene) - [Pull Request Process](#pull-request-process) - [Issue Reporting](#issue-reporting) - [Architecture Overview](#architecture-overview) @@ -617,6 +618,41 @@ git commit -m "WIP" - **body**: Detailed explanation if needed (wrap at 72 chars) - **footer**: Reference issues, breaking changes +### PR Hygiene + +**Rebasing:** +- **Rebase onto develop** before opening a PR and before merge to maintain linear history +- Use `git fetch origin && git rebase origin/develop` to sync your branch +- Use `--force-with-lease` when force-pushing rebased branches (safer than `--force`) +- Notify reviewers after force-pushing during active review +- **Exception:** Never rebase after PR is approved and others have reviewed specific commits + +**Commit organization:** +- **Squash fixup commits** (typos, "oops", review feedback) into their parent commits +- **Keep logically distinct changes** as separate commits that could be reverted independently +- Each commit should compile and pass tests independently +- No "WIP", "fix tests", or "lint" commits in final PR - squash these + +**Before requesting review:** +```bash +# Ensure up-to-date with develop +git fetch origin && git rebase origin/develop + +# Clean up commit history (squash fixups, reword messages) +git rebase -i origin/develop + +# Force push with safety check +git push --force-with-lease + +# Verify everything works +npm run test:backend +cd apps/frontend && npm test && npm run lint && npm run typecheck +``` + +**PR size:** +- Keep PRs small (<400 lines changed ideally) +- Split large features into stacked PRs if possible + ## Pull Request Process 1. **Fork the repository** and create your branch from `develop` (not main!) diff --git a/apps/backend/core/client.py b/apps/backend/core/client.py index 310842cd..b3015be6 100644 --- a/apps/backend/core/client.py +++ b/apps/backend/core/client.py @@ -139,6 +139,7 @@ def create_client( model: str, agent_type: str = "coder", max_thinking_tokens: int | None = None, + output_format: dict | None = None, ) -> ClaudeSDKClient: """ Create a Claude Agent SDK client with multi-layered security. @@ -154,6 +155,9 @@ def create_client( - high: 10000 (QA review) - medium: 5000 (planning, validation) - None: disabled (coding) + output_format: Optional structured output format for validated JSON responses. + Use {"type": "json_schema", "schema": Model.model_json_schema()} + See: https://platform.claude.com/docs/en/agent-sdk/structured-outputs Returns: Configured ClaudeSDKClient @@ -339,30 +343,36 @@ def create_client( if auto_claude_mcp_server: mcp_servers["auto-claude"] = auto_claude_mcp_server - return ClaudeSDKClient( - options=ClaudeAgentOptions( - model=model, - system_prompt=( - f"You are an expert full-stack developer building production-quality software. " - f"Your working directory is: {project_dir.resolve()}\n" - f"Your filesystem access is RESTRICTED to this directory only. " - f"Use relative paths (starting with ./) for all file operations. " - f"Never use absolute paths or try to access files outside your working directory.\n\n" - f"You follow existing code patterns, write clean maintainable code, and verify " - f"your work through thorough testing. You communicate progress through Git commits " - f"and build-progress.txt updates." - ), - allowed_tools=allowed_tools_list, - mcp_servers=mcp_servers, - hooks={ - "PreToolUse": [ - HookMatcher(matcher="Bash", hooks=[bash_security_hook]), - ], - }, - max_turns=1000, - cwd=str(project_dir.resolve()), - settings=str(settings_file.resolve()), - env=sdk_env, # Pass ANTHROPIC_BASE_URL etc. to subprocess - max_thinking_tokens=max_thinking_tokens, # Extended thinking budget - ) - ) + # Build options dict, conditionally including output_format + options_kwargs = { + "model": model, + "system_prompt": ( + f"You are an expert full-stack developer building production-quality software. " + f"Your working directory is: {project_dir.resolve()}\n" + f"Your filesystem access is RESTRICTED to this directory only. " + f"Use relative paths (starting with ./) for all file operations. " + f"Never use absolute paths or try to access files outside your working directory.\n\n" + f"You follow existing code patterns, write clean maintainable code, and verify " + f"your work through thorough testing. You communicate progress through Git commits " + f"and build-progress.txt updates." + ), + "allowed_tools": allowed_tools_list, + "mcp_servers": mcp_servers, + "hooks": { + "PreToolUse": [ + HookMatcher(matcher="Bash", hooks=[bash_security_hook]), + ], + }, + "max_turns": 1000, + "cwd": str(project_dir.resolve()), + "settings": str(settings_file.resolve()), + "env": sdk_env, # Pass ANTHROPIC_BASE_URL etc. to subprocess + "max_thinking_tokens": max_thinking_tokens, # Extended thinking budget + } + + # Add structured output format if specified + # See: https://platform.claude.com/docs/en/agent-sdk/structured-outputs + if output_format: + options_kwargs["output_format"] = output_format + + return ClaudeSDKClient(options=ClaudeAgentOptions(**options_kwargs)) diff --git a/apps/backend/requirements.txt b/apps/backend/requirements.txt index 54258fe2..59aec7b0 100644 --- a/apps/backend/requirements.txt +++ b/apps/backend/requirements.txt @@ -12,3 +12,6 @@ graphiti-core>=0.5.0; python_version >= "3.12" # Google AI (optional - for Gemini LLM and embeddings) google-generativeai>=0.8.0 + +# Pydantic for structured output schemas +pydantic>=2.0.0 diff --git a/apps/backend/runners/github/context_gatherer.py b/apps/backend/runners/github/context_gatherer.py index 00374bbd..a015b257 100644 --- a/apps/backend/runners/github/context_gatherer.py +++ b/apps/backend/runners/github/context_gatherer.py @@ -28,6 +28,45 @@ try: except (ImportError, ValueError, SystemError): from gh_client import GHClient, PRTooLargeError +# Validation patterns for git refs and paths (defense-in-depth) +# These patterns allow common valid characters while rejecting potentially dangerous ones +SAFE_REF_PATTERN = re.compile(r"^[a-zA-Z0-9._/\-]+$") +SAFE_PATH_PATTERN = re.compile(r"^[a-zA-Z0-9._/\-@]+$") + + +def _validate_git_ref(ref: str) -> bool: + """ + Validate git ref (branch name or commit SHA) for safe use in commands. + + Args: + ref: Git ref to validate + + Returns: + True if ref is safe, False otherwise + """ + if not ref or len(ref) > 256: + return False + return bool(SAFE_REF_PATTERN.match(ref)) + + +def _validate_file_path(path: str) -> bool: + """ + Validate file path for safe use in git commands. + + Args: + path: File path to validate + + Returns: + True if path is safe, False otherwise + """ + if not path or len(path) > 1024: + return False + # Reject path traversal attempts + if ".." in path or path.startswith("/"): + return False + return bool(SAFE_PATH_PATTERN.match(path)) + + if TYPE_CHECKING: try: from .models import FollowupReviewContext, PRReviewResult @@ -139,6 +178,19 @@ class PRContextGatherer: flush=True, ) + # Ensure PR refs are available locally (fetches commits for fork PRs) + head_sha = pr_data.get("headRefOid", "") + base_sha = pr_data.get("baseRefOid", "") + refs_available = False + if head_sha and base_sha: + refs_available = await self._ensure_pr_refs_available(head_sha, base_sha) + if not refs_available: + print( + "[Context] Warning: Could not fetch PR refs locally. " + "Will use GitHub API patches as fallback.", + flush=True, + ) + # Fetch changed files with content changed_files = await self._fetch_changed_files(pr_data) print(f"[Context] Fetched {len(changed_files)} changed files", flush=True) @@ -197,6 +249,8 @@ class PRContextGatherer: "state", "headRefName", "baseRefName", + "headRefOid", # Commit SHA for head - works even when branch is unavailable locally + "baseRefOid", # Commit SHA for base - works even when branch is unavailable locally "author", "files", "additions", @@ -206,6 +260,83 @@ class PRContextGatherer: ], ) + async def _ensure_pr_refs_available(self, head_sha: str, base_sha: str) -> bool: + """ + Ensure PR refs are available locally by fetching the commit SHAs. + + This solves the "fatal: bad revision" error when PR branches aren't + available locally (e.g., PRs from forks or unfetched branches). + + Args: + head_sha: The head commit SHA (from headRefOid) + base_sha: The base commit SHA (from baseRefOid) + + Returns: + True if refs are available, False otherwise + """ + # Validate SHAs before using in git commands + if not _validate_git_ref(head_sha): + print( + f"[Context] Invalid head SHA rejected: {head_sha[:50]}...", flush=True + ) + return False + if not _validate_git_ref(base_sha): + print( + f"[Context] Invalid base SHA rejected: {base_sha[:50]}...", flush=True + ) + return False + + try: + # Fetch the specific commits - this works even for fork PRs + proc = await asyncio.create_subprocess_exec( + "git", + "fetch", + "origin", + head_sha, + base_sha, + cwd=self.project_dir, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + ) + stdout, stderr = await asyncio.wait_for(proc.communicate(), timeout=30.0) + + if proc.returncode == 0: + print( + f"[Context] Fetched PR refs: {head_sha[:8]}...{base_sha[:8]}", + flush=True, + ) + return True + else: + # If direct SHA fetch fails, try fetching the PR ref + print("[Context] Direct SHA fetch failed, trying PR ref...", flush=True) + proc2 = await asyncio.create_subprocess_exec( + "git", + "fetch", + "origin", + f"pull/{self.pr_number}/head:refs/pr/{self.pr_number}", + cwd=self.project_dir, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + ) + await asyncio.wait_for(proc2.communicate(), timeout=30.0) + if proc2.returncode == 0: + print( + f"[Context] Fetched PR ref: refs/pr/{self.pr_number}", + flush=True, + ) + return True + print( + f"[Context] Failed to fetch PR refs: {stderr.decode('utf-8')}", + flush=True, + ) + return False + except asyncio.TimeoutError: + print("[Context] Timeout fetching PR refs", flush=True) + return False + except Exception as e: + print(f"[Context] Error fetching PR refs: {e}", flush=True) + return False + async def _fetch_changed_files(self, pr_data: dict) -> list[ChangedFile]: """ Fetch all changed files with their full content. @@ -226,16 +357,18 @@ class PRContextGatherer: print(f"[Context] Processing {path} ({status})...", flush=True) - # Get current content (from PR head branch) - content = await self._read_file_content(path, pr_data["headRefName"]) + # Use commit SHAs if available (works for fork PRs), fallback to branch names + head_ref = pr_data.get("headRefOid") or pr_data["headRefName"] + base_ref = pr_data.get("baseRefOid") or pr_data["baseRefName"] - # Get base content (from base branch) - base_content = await self._read_file_content(path, pr_data["baseRefName"]) + # Get current content (from PR head commit) + content = await self._read_file_content(path, head_ref) + + # Get base content (from base commit) + base_content = await self._read_file_content(path, base_ref) # Get the patch for this specific file - patch = await self._get_file_patch( - path, pr_data["baseRefName"], pr_data["headRefName"] - ) + patch = await self._get_file_patch(path, base_ref, head_ref) changed_files.append( ChangedFile( @@ -276,6 +409,14 @@ class PRContextGatherer: Returns: File content as string, or empty string if file doesn't exist """ + # Validate inputs to prevent command injection + if not _validate_file_path(path): + print(f"[Context] Invalid file path rejected: {path[:50]}...", flush=True) + return "" + if not _validate_git_ref(ref): + print(f"[Context] Invalid git ref rejected: {ref[:50]}...", flush=True) + return "" + try: proc = await asyncio.create_subprocess_exec( "git", @@ -312,6 +453,21 @@ class PRContextGatherer: Returns: Unified diff patch for this file """ + # Validate inputs to prevent command injection + if not _validate_file_path(path): + print(f"[Context] Invalid file path rejected: {path[:50]}...", flush=True) + return "" + if not _validate_git_ref(base_ref): + print( + f"[Context] Invalid base ref rejected: {base_ref[:50]}...", flush=True + ) + return "" + if not _validate_git_ref(head_ref): + print( + f"[Context] Invalid head ref rejected: {head_ref[:50]}...", flush=True + ) + return "" + try: proc = await asyncio.create_subprocess_exec( "git", diff --git a/apps/backend/runners/github/services/followup_reviewer.py b/apps/backend/runners/github/services/followup_reviewer.py index 240cd068..e7bfeb74 100644 --- a/apps/backend/runners/github/services/followup_reviewer.py +++ b/apps/backend/runners/github/services/followup_reviewer.py @@ -16,7 +16,6 @@ Supports both: from __future__ import annotations import hashlib -import json import logging import re from datetime import datetime @@ -35,6 +34,7 @@ try: ReviewSeverity, ) from .prompt_manager import PromptManager + from .pydantic_models import FollowupReviewResponse except (ImportError, ValueError, SystemError): from models import ( MergeVerdict, @@ -44,18 +44,38 @@ except (ImportError, ValueError, SystemError): ReviewSeverity, ) from services.prompt_manager import PromptManager + from services.pydantic_models import FollowupReviewResponse logger = logging.getLogger(__name__) # Category mapping for AI responses _CATEGORY_MAPPING = { + # Direct matches (already valid) "security": ReviewCategory.SECURITY, "quality": ReviewCategory.QUALITY, - "logic": ReviewCategory.QUALITY, + "style": ReviewCategory.STYLE, "test": ReviewCategory.TEST, "docs": ReviewCategory.DOCS, "pattern": ReviewCategory.PATTERN, "performance": ReviewCategory.PERFORMANCE, + "verification_failed": ReviewCategory.VERIFICATION_FAILED, + "redundancy": ReviewCategory.REDUNDANCY, + # AI-generated alternatives that need mapping + "correctness": ReviewCategory.QUALITY, # Logic/code correctness → quality + "consistency": ReviewCategory.PATTERN, # Code consistency → pattern adherence + "testing": ReviewCategory.TEST, # Testing → test + "documentation": ReviewCategory.DOCS, # Documentation → docs + "bug": ReviewCategory.QUALITY, # Bug → quality + "logic": ReviewCategory.QUALITY, # Logic error → quality + "error_handling": ReviewCategory.QUALITY, # Error handling → quality + "maintainability": ReviewCategory.QUALITY, # Maintainability → quality + "readability": ReviewCategory.STYLE, # Readability → style + "best_practices": ReviewCategory.PATTERN, # Best practices → pattern + "best-practices": ReviewCategory.PATTERN, # With hyphen + "architecture": ReviewCategory.PATTERN, # Architecture → pattern + "complexity": ReviewCategory.QUALITY, # Complexity → quality + "dead_code": ReviewCategory.REDUNDANCY, # Dead code → redundancy + "unused": ReviewCategory.REDUNDANCY, # Unused → redundancy } # Severity mapping for AI responses @@ -291,11 +311,15 @@ class FollowupReviewer: return resolved, unresolved - def _line_appears_changed(self, file: str, line: int, diff: str) -> bool: + def _line_appears_changed(self, file: str, line: int | None, diff: str) -> bool: """Check if a specific line appears to have been changed in the diff.""" if not diff: return False + # Handle None or invalid line numbers (legacy data) + if line is None or line <= 0: + return True # Assume changed if line unknown + # Look for the file in the diff file_marker = f"--- a/{file}" if file_marker not in diff: @@ -538,7 +562,10 @@ class FollowupReviewer: unresolved: list[PRReviewFinding], ) -> dict[str, Any] | None: """ - Run AI-powered follow-up review using the prompt template. + Run AI-powered follow-up review using structured outputs. + + Uses Claude Agent SDK's native structured output support to guarantee + valid JSON responses matching the FollowupReviewResponse schema. Returns parsed AI response with finding resolutions and new findings, or None if AI review fails. @@ -548,7 +575,7 @@ class FollowupReviewer: ) # Build the context for the AI - prompt = self.prompt_manager.get_followup_review_prompt() + prompt_template = self.prompt_manager.get_followup_review_prompt() # Format previous findings for the prompt previous_findings_text = "\n".join( @@ -583,7 +610,7 @@ class FollowupReviewer: # Build the full message user_message = f""" -{prompt} +{prompt_template} --- @@ -615,49 +642,77 @@ class FollowupReviewer: --- -Please analyze this follow-up review context and provide your response in the JSON format specified in the prompt. +Analyze this follow-up review context and provide your structured response. """ try: - # Use ClaudeSDKClient directly for simple message calls - # (no agent tools needed, just a single query/response) - from claude_agent_sdk import ClaudeAgentOptions, ClaudeSDKClient + # Use Claude Agent SDK query() with structured outputs + # Reference: https://platform.claude.com/docs/en/agent-sdk/structured-outputs + from claude_agent_sdk import ClaudeAgentOptions, query model = self.config.model or "claude-sonnet-4-5-20250929" - client = ClaudeSDKClient( + # Debug: Log the schema being sent + schema = FollowupReviewResponse.model_json_schema() + logger.debug( + f"[Followup] Using output_format schema: {list(schema.get('properties', {}).keys())}" + ) + print(f"[Followup] SDK query with output_format, model={model}", flush=True) + + # Iterate through messages from the query + # Note: max_turns=2 because structured output uses a tool call + response + async for message in query( + prompt=user_message, options=ClaudeAgentOptions( model=model, - system_prompt="You are a code review assistant. Analyze the provided context and respond with valid JSON.", + system_prompt="You are a code review assistant. Analyze the provided context and provide structured feedback.", allowed_tools=[], - max_turns=1, + max_turns=2, # Need 2 turns for structured output tool call max_thinking_tokens=2048, - ) - ) + output_format={ + "type": "json_schema", + "schema": schema, + }, + ), + ): + msg_type = type(message).__name__ - response_text = "" - async with client: - await client.query(user_message) + # SDK delivers structured output via ToolUseBlock named 'StructuredOutput' + # in an AssistantMessage + if msg_type == "AssistantMessage": + content = getattr(message, "content", []) + for block in content: + block_type = type(block).__name__ + if block_type == "ToolUseBlock": + tool_name = getattr(block, "name", "") + if tool_name == "StructuredOutput": + # Extract structured data from tool input + structured_data = getattr(block, "input", None) + if structured_data: + logger.info( + "[Followup] Found StructuredOutput tool use" + ) + print( + "[Followup] Using SDK structured output", + flush=True, + ) + # Validate with Pydantic and convert + result = FollowupReviewResponse.model_validate( + structured_data + ) + return self._convert_structured_to_internal(result) - async for msg in client.receive_response(): - msg_type = type(msg).__name__ - logger.debug(f"AI response message type: {msg_type}") - if msg_type == "AssistantMessage" and hasattr(msg, "content"): - for block in msg.content: - block_type = type(block).__name__ - logger.debug(f" Content block type: {block_type}") - if hasattr(block, "text"): - response_text += block.text - elif hasattr(block, "thinking"): - # Skip thinking blocks - we only want the final text - logger.debug(" (skipping thinking block)") + # Handle ResultMessage for errors + if msg_type == "ResultMessage": + subtype = getattr(message, "subtype", None) + if subtype == "error_max_structured_output_retries": + logger.warning( + "Claude could not produce valid structured output after retries" + ) + return None - if not response_text: - logger.warning("AI returned empty response (no text blocks found)") - return None - - logger.debug(f"AI response text (first 500 chars): {response_text[:500]}") - return self._parse_ai_response(response_text) + logger.warning("No structured output received from AI") + return None except ValueError as e: # OAuth token not found @@ -665,96 +720,69 @@ Please analyze this follow-up review context and provide your response in the JS print("AI review failed: No OAuth token found", flush=True) return None except Exception as e: - logger.error(f"AI review failed: {e}") + logger.error(f"AI review with structured output failed: {e}") return None - def _parse_ai_response(self, response_text: str) -> dict[str, Any] | None: - """Parse the AI response JSON.""" - # Extract JSON from response (may be wrapped in markdown code blocks) - json_match = re.search(r"```json\s*(.*?)\s*```", response_text, re.DOTALL) - if json_match: - json_str = json_match.group(1) - else: - # Try to find raw JSON - json_match = re.search(r"\{[\s\S]*\}", response_text) - if json_match: - json_str = json_match.group(0) - else: - logger.warning("No JSON found in AI response") - return None + def _convert_structured_to_internal( + self, result: FollowupReviewResponse + ) -> dict[str, Any]: + """ + Convert Pydantic FollowupReviewResponse to internal dict format. - try: - data = json.loads(json_str) + Converts Pydantic finding models to PRReviewFinding dataclass objects + for compatibility with existing codebase. + """ + # Convert new_findings to PRReviewFinding objects + new_findings = [] + for f in result.new_findings: + new_findings.append( + PRReviewFinding( + id=f.id, + severity=_SEVERITY_MAPPING.get(f.severity, ReviewSeverity.MEDIUM), + category=_CATEGORY_MAPPING.get(f.category, ReviewCategory.QUALITY), + title=f.title, + description=f.description, + file=f.file, + line=f.line, + suggested_fix=f.suggested_fix, + fixable=f.fixable, + ) + ) - # Convert new_findings to PRReviewFinding objects - new_findings = [] - for f in data.get("new_findings", []): - try: - new_findings.append( - PRReviewFinding( - id=f.get( - "id", - hashlib.md5( - f.get("title", "").encode(), usedforsecurity=False - ).hexdigest()[:12], - ), - severity=_SEVERITY_MAPPING.get( - f.get("severity", "medium").lower(), - ReviewSeverity.MEDIUM, - ), - category=_CATEGORY_MAPPING.get( - f.get("category", "quality").lower(), - ReviewCategory.QUALITY, - ), - title=f.get("title", "Untitled finding"), - description=f.get("description", ""), - file=f.get("file", ""), - line=f.get("line", 0), - suggested_fix=f.get("suggested_fix"), - ) - ) - except Exception as e: - logger.warning(f"Failed to parse finding: {e}") + # Convert comment_findings to PRReviewFinding objects + comment_findings = [] + for f in result.comment_findings: + comment_findings.append( + PRReviewFinding( + id=f.id, + severity=_SEVERITY_MAPPING.get(f.severity, ReviewSeverity.LOW), + category=_CATEGORY_MAPPING.get(f.category, ReviewCategory.QUALITY), + title=f.title, + description=f.description, + file=f.file, + line=f.line, + suggested_fix=f.suggested_fix, + fixable=f.fixable, + ) + ) - # Convert comment_findings similarly - comment_findings = [] - for f in data.get("comment_findings", []): - try: - comment_findings.append( - PRReviewFinding( - id=f.get( - "id", - hashlib.md5( - f.get("title", "").encode(), usedforsecurity=False - ).hexdigest()[:12], - ), - severity=_SEVERITY_MAPPING.get( - f.get("severity", "low").lower(), ReviewSeverity.LOW - ), - category=_CATEGORY_MAPPING.get( - f.get("category", "quality").lower(), - ReviewCategory.QUALITY, - ), - title=f.get("title", "Comment needs attention"), - description=f.get("description", ""), - file=f.get("file", ""), - line=f.get("line", 0), - ) - ) - except Exception as e: - logger.warning(f"Failed to parse comment finding: {e}") - - return { - "finding_resolutions": data.get("finding_resolutions", []), - "new_findings": new_findings, - "comment_findings": comment_findings, - "verdict": data.get("verdict"), - "verdict_reasoning": data.get("verdict_reasoning"), + # Convert finding_resolutions to dict format + finding_resolutions = [ + { + "finding_id": r.finding_id, + "status": r.status, + "resolution_notes": r.resolution_notes, } + for r in result.finding_resolutions + ] - except json.JSONDecodeError as e: - logger.error(f"Failed to parse AI response JSON: {e}") - return None + return { + "finding_resolutions": finding_resolutions, + "new_findings": new_findings, + "comment_findings": comment_findings, + "verdict": result.verdict, + "verdict_reasoning": result.verdict_reasoning, + } def _apply_ai_resolutions( self, diff --git a/apps/backend/runners/github/services/orchestrator_reviewer.py b/apps/backend/runners/github/services/orchestrator_reviewer.py index b7b8a873..f20db718 100644 --- a/apps/backend/runners/github/services/orchestrator_reviewer.py +++ b/apps/backend/runners/github/services/orchestrator_reviewer.py @@ -12,9 +12,13 @@ from __future__ import annotations import json import logging +import os from pathlib import Path from typing import Any +# Check if debug mode is enabled (via DEBUG=true env var) +DEBUG_MODE = os.environ.get("DEBUG", "").lower() in ("true", "1", "yes") + try: from ...core.client import create_client from ..context_gatherer import PRContext @@ -26,6 +30,7 @@ try: ReviewCategory, ReviewSeverity, ) + from .pydantic_models import OrchestratorReviewResponse from .review_tools import ( check_coverage, get_file_content, @@ -46,6 +51,7 @@ except (ImportError, ValueError, SystemError): ReviewCategory, ReviewSeverity, ) + from services.pydantic_models import OrchestratorReviewResponse from services.review_tools import ( check_coverage, get_file_content, @@ -185,6 +191,10 @@ class OrchestratorReviewer: model="claude-opus-4-5-20251101", # Opus for strategic thinking agent_type="pr_reviewer", # Read-only - no bash, no edits max_thinking_tokens=10000, # High budget for strategy + output_format={ + "type": "json_schema", + "schema": OrchestratorReviewResponse.model_json_schema(), + }, ) self._report_progress( @@ -199,6 +209,7 @@ class OrchestratorReviewer: test_result = None result_text = "" tool_calls_made = [] + structured_output = None # For SDK structured outputs logger.info(f"[Orchestrator] Sending prompt (length: {len(prompt)} chars)") logger.debug(f"[Orchestrator] Prompt preview: {prompt[:500]}...") @@ -206,10 +217,89 @@ class OrchestratorReviewer: async with client: await client.query(prompt) + print( + "[Orchestrator] Waiting for LLM response (Opus 4.5 with extended thinking)...", + flush=True, + ) + if DEBUG_MODE: + print( + "[DEBUG Orchestrator] Starting to receive LLM response stream...", + flush=True, + ) + + message_count = 0 + thinking_received = False + text_started = False + async for msg in client.receive_response(): msg_type = type(msg).__name__ + message_count += 1 logger.debug(f"[Orchestrator] Received message type: {msg_type}") + # DEBUG: Log all message types received + if DEBUG_MODE: + print( + f"[DEBUG Orchestrator] Received message #{message_count}: {msg_type}", + flush=True, + ) + + # Handle extended thinking blocks (shows LLM reasoning) + if msg_type == "ThinkingBlock" or ( + hasattr(msg, "type") and msg.type == "thinking" + ): + if not thinking_received: + print( + "[Orchestrator] LLM is thinking (extended thinking)...", + flush=True, + ) + thinking_received = True + + thinking_text = ( + msg.thinking + if hasattr(msg, "thinking") + else getattr(msg, "text", "") + ) + if DEBUG_MODE and thinking_text: + print( + "[DEBUG Orchestrator] ===== LLM THINKING START =====", + flush=True, + ) + # Print thinking in chunks to avoid buffer issues + for i in range(0, len(thinking_text), 1000): + print(thinking_text[i : i + 1000], flush=True) + print( + "[DEBUG Orchestrator] ===== LLM THINKING END =====", + flush=True, + ) + else: + # Even without DEBUG, show thinking length + print( + f"[Orchestrator] Thinking block received ({len(thinking_text)} chars)", + flush=True, + ) + logger.debug( + f"[Orchestrator] Thinking block (length: {len(thinking_text)})" + ) + + # Handle text delta streaming (real-time output) + if msg_type == "TextDelta" or ( + hasattr(msg, "type") and msg.type == "text_delta" + ): + if not text_started: + print( + "[Orchestrator] LLM is generating response...", + flush=True, + ) + text_started = True + + delta_text = ( + msg.text + if hasattr(msg, "text") + else getattr(msg, "delta", "") + ) + if DEBUG_MODE and delta_text: + print(delta_text, end="", flush=True) + # Handle tool calls from orchestrator if msg_type == "ToolUseBlock" or ( hasattr(msg, "type") and msg.type == "tool_use" @@ -224,6 +314,20 @@ class OrchestratorReviewer: tool_calls_made.append(tool_name) logger.info(f"[Orchestrator] Tool call detected: {tool_name}") + # SDK delivers structured output via StructuredOutput tool + if tool_name == "StructuredOutput": + structured_data = getattr(msg, "input", None) + if structured_data: + structured_output = structured_data + logger.info( + "[Orchestrator] Found StructuredOutput tool use" + ) + print( + "[Orchestrator] Received SDK structured output", + flush=True, + ) + continue # No need to handle as regular tool + tool_result = await self._handle_tool_call(msg, context) # Tools already executed, agent will receive results @@ -259,11 +363,33 @@ class OrchestratorReviewer: # Collect final orchestrator output if msg_type == "AssistantMessage" and hasattr(msg, "content"): for block in msg.content: + block_type = type(block).__name__ if hasattr(block, "text"): result_text += block.text logger.debug( f"[Orchestrator] Received text block (length: {len(block.text)})" ) + # Also check for StructuredOutput in AssistantMessage content + if block_type == "ToolUseBlock": + tool_name = getattr(block, "name", "") + if tool_name == "StructuredOutput": + structured_data = getattr(block, "input", None) + if structured_data: + structured_output = structured_data + logger.info( + "[Orchestrator] Found StructuredOutput in AssistantMessage" + ) + print( + "[Orchestrator] Received SDK structured output", + flush=True, + ) + + # Check for structured output (SDK validated JSON) + if hasattr(msg, "structured_output") and msg.structured_output: + structured_output = msg.structured_output + logger.info( + "[Orchestrator] Received structured output from SDK" + ) logger.info( f"[Orchestrator] Session complete. Tool calls made: {tool_calls_made}" @@ -293,8 +419,28 @@ class OrchestratorReviewer: pr_number=context.pr_number, ) - # Parse orchestrator's final output - orchestrator_findings = self._parse_orchestrator_output(result_text) + # Use structured output if available, otherwise fall back to parsing + if structured_output: + logger.info("[Orchestrator] Using validated structured output") + print("[Orchestrator] Using SDK structured output", flush=True) + orchestrator_findings = self._parse_structured_output(structured_output) + # Fallback to text parsing only if structured output parsing FAILED (None) + # An empty list means the PR is clean - don't trigger fallback + if orchestrator_findings is None and result_text: + logger.warning( + "[Orchestrator] Structured output parsing failed, falling back to text" + ) + print( + "[Orchestrator] Structured output failed, trying text parsing fallback", + flush=True, + ) + orchestrator_findings = self._parse_orchestrator_output(result_text) + elif orchestrator_findings is None: + orchestrator_findings = [] # No fallback available, use empty + else: + logger.info("[Orchestrator] Falling back to text parsing") + print("[Orchestrator] Falling back to text parsing", flush=True) + orchestrator_findings = self._parse_orchestrator_output(result_text) all_findings.extend(orchestrator_findings) # Deduplicate findings @@ -548,6 +694,75 @@ Now perform your strategic review and use the available tools to spawn subagents return base_prompt + pr_context + def _parse_structured_output( + self, structured_output: dict[str, Any] + ) -> list[PRReviewFinding] | None: + """ + Parse findings from SDK structured output. + + Uses the validated OrchestratorReviewResponse schema for type-safe parsing. + + Returns: + List of findings on success (may be empty for clean PRs), + None on parsing failure (triggers fallback to text parsing). + """ + findings = [] + + try: + # Validate with Pydantic + result = OrchestratorReviewResponse.model_validate(structured_output) + + logger.info( + f"[Orchestrator] Structured output: verdict={result.verdict}, " + f"{len(result.findings)} findings" + ) + + for f in result.findings: + # Generate unique ID for this finding + import hashlib + + finding_id = hashlib.md5( + f"{f.file}:{f.line}:{f.title}".encode(), + usedforsecurity=False, + ).hexdigest()[:12] + + # Map category using flexible mapping + category = _map_category(f.category) + + # Map severity + try: + severity = ReviewSeverity(f.severity.lower()) + except ValueError: + severity = ReviewSeverity.MEDIUM + + finding = PRReviewFinding( + id=finding_id, + file=f.file, + line=f.line, + title=f.title, + description=f.description, + category=category, + severity=severity, + suggested_fix=f.suggestion or "", + confidence=self._normalize_confidence(f.confidence), + ) + findings.append(finding) + logger.debug( + f"[Orchestrator] Added structured finding: {finding.title} ({finding.severity.value})" + ) + + print( + f"[Orchestrator] Processed {len(findings)} findings from structured output", + flush=True, + ) + + except Exception as e: + logger.error(f"[Orchestrator] Failed to parse structured output: {e}") + print(f"[Orchestrator] Structured output parsing failed: {e}", flush=True) + return None # Signal failure - triggers fallback to text parsing + + return findings + def _parse_orchestrator_output(self, output: str) -> list[PRReviewFinding]: """Parse findings from orchestrator's final output.""" findings = [] @@ -666,9 +881,9 @@ Now perform your strategic review and use the available tools to spawn subagents suggested_fix=data.get( "suggestion", data.get("suggested_fix", "") ), - confidence=data.get("confidence", 85) / 100.0 - if data.get("confidence", 85) > 1 - else data.get("confidence", 0.85), + confidence=self._normalize_confidence( + data.get("confidence", 85) + ), ) findings.append(finding) logger.debug( @@ -733,9 +948,9 @@ Now perform your strategic review and use the available tools to spawn subagents suggested_fix=data.get( "suggestion", data.get("suggested_fix", "") ), - confidence=data.get("confidence", 85) / 100.0 - if data.get("confidence", 85) > 1 - else data.get("confidence", 0.85), + confidence=self._normalize_confidence( + data.get("confidence", 85) + ), ) findings.append(finding) logger.debug( @@ -751,6 +966,27 @@ Now perform your strategic review and use the available tools to spawn subagents logger.info(f"[Orchestrator] Parsed {len(findings)} total findings from output") return findings + def _normalize_confidence(self, confidence_value: int | float) -> float: + """ + Normalize confidence value to 0.0-1.0 range. + + AI models may return confidence as: + - Percentage (0-100): divide by 100 + - Decimal (0.0-1.0): use as-is + + Args: + confidence_value: Raw confidence value from AI output + + Returns: + Normalized confidence as float in 0.0-1.0 range + """ + if confidence_value > 1: + # Percentage format (e.g., 85 -> 0.85) + return confidence_value / 100.0 + else: + # Already decimal format (e.g., 0.85) + return float(confidence_value) + def _extract_findings_from_data( self, findings_data: list[dict] ) -> list[PRReviewFinding]: @@ -791,9 +1027,7 @@ Now perform your strategic review and use the available tools to spawn subagents category=category, severity=severity, suggested_fix=data.get("suggestion", data.get("suggested_fix", "")), - confidence=data.get("confidence", 85) / 100.0 - if data.get("confidence", 85) > 1 - else data.get("confidence", 0.85), + confidence=self._normalize_confidence(data.get("confidence", 85)), ) findings.append(finding) logger.debug( diff --git a/apps/backend/runners/github/services/pydantic_models.py b/apps/backend/runners/github/services/pydantic_models.py new file mode 100644 index 00000000..0c50bffe --- /dev/null +++ b/apps/backend/runners/github/services/pydantic_models.py @@ -0,0 +1,344 @@ +""" +Pydantic Models for Structured AI Outputs +========================================== + +These models define JSON schemas for Claude Agent SDK structured outputs. +Used to guarantee valid, validated JSON from AI responses in PR reviews. + +Usage: + from claude_agent_sdk import query + from .pydantic_models import FollowupReviewResponse + + async for message in query( + prompt="...", + options={ + "output_format": { + "type": "json_schema", + "schema": FollowupReviewResponse.model_json_schema() + } + } + ): + if hasattr(message, 'structured_output'): + result = FollowupReviewResponse.model_validate(message.structured_output) +""" + +from __future__ import annotations + +from typing import Literal + +from pydantic import BaseModel, Field, field_validator + +# ============================================================================= +# Common Finding Types +# ============================================================================= + + +class BaseFinding(BaseModel): + """Base class for all finding types.""" + + id: str = Field(description="Unique identifier for this finding") + severity: Literal["critical", "high", "medium", "low"] = Field( + description="Issue severity level" + ) + title: str = Field(description="Brief issue title (max 80 chars)") + description: str = Field(description="Detailed explanation of the issue") + file: str = Field(description="File path where issue was found") + line: int = Field(0, description="Line number of the issue") + suggested_fix: str | None = Field(None, description="How to fix this issue") + fixable: bool = Field(False, description="Whether this can be auto-fixed") + + +class SecurityFinding(BaseFinding): + """A security vulnerability finding.""" + + category: Literal["security"] = Field( + default="security", description="Always 'security' for security findings" + ) + + +class QualityFinding(BaseFinding): + """A code quality or redundancy finding.""" + + category: Literal[ + "redundancy", "quality", "test", "performance", "pattern", "docs" + ] = Field(description="Issue category") + redundant_with: str | None = Field( + None, description="Reference to duplicate code (file:line) if redundant" + ) + + +class DeepAnalysisFinding(BaseFinding): + """A finding from deep analysis with verification info.""" + + category: Literal[ + "verification_failed", + "redundancy", + "quality", + "pattern", + "performance", + "logic", + ] = Field(description="Issue category") + confidence: float = Field( + 0.85, ge=0.0, le=1.0, description="AI's confidence in this finding (0.0-1.0)" + ) + verification_note: str | None = Field( + None, description="What evidence is missing or couldn't be verified" + ) + + +class StructuralIssue(BaseModel): + """A structural issue with the PR.""" + + id: str = Field(description="Unique identifier") + issue_type: Literal[ + "feature_creep", "scope_creep", "architecture_violation", "poor_structure" + ] = Field(description="Type of structural issue") + severity: Literal["critical", "high", "medium", "low"] = Field( + description="Issue severity" + ) + title: str = Field(description="Brief issue title") + description: str = Field(description="Detailed explanation") + impact: str = Field(description="Why this matters") + suggestion: str = Field(description="How to fix") + + +class AICommentTriage(BaseModel): + """Triage result for an AI tool comment.""" + + comment_id: int = Field(description="GitHub comment ID") + tool_name: str = Field( + description="AI tool name (CodeRabbit, Cursor, Greptile, etc.)" + ) + verdict: Literal[ + "critical", "important", "nice_to_have", "trivial", "false_positive" + ] = Field(description="Verdict on the comment") + reasoning: str = Field(description="Why this verdict was chosen") + response_comment: str | None = Field( + None, description="Optional comment to post in reply" + ) + + +# ============================================================================= +# Follow-up Review Response +# ============================================================================= + + +class FindingResolution(BaseModel): + """Resolution status for a previous finding.""" + + finding_id: str = Field(description="ID of the previous finding") + status: Literal["resolved", "unresolved"] = Field(description="Resolution status") + resolution_notes: str | None = Field( + None, description="Notes on how it was resolved" + ) + + +class FollowupFinding(BaseModel): + """A new finding from follow-up review (simpler than initial review).""" + + id: str = Field(description="Unique identifier for this finding") + severity: Literal["critical", "high", "medium", "low"] = Field( + description="Issue severity level" + ) + category: Literal["security", "quality", "logic", "test", "docs"] = Field( + description="Issue category" + ) + title: str = Field(description="Brief issue title") + description: str = Field(description="Detailed explanation of the issue") + file: str = Field(description="File path where issue was found") + line: int = Field(0, description="Line number of the issue") + suggested_fix: str | None = Field(None, description="How to fix this issue") + fixable: bool = Field(False, description="Whether this can be auto-fixed") + + +class FollowupReviewResponse(BaseModel): + """Complete response schema for follow-up PR review.""" + + finding_resolutions: list[FindingResolution] = Field( + default_factory=list, description="Status of each previous finding" + ) + new_findings: list[FollowupFinding] = Field( + default_factory=list, + description="New issues found in changes since last review", + ) + comment_findings: list[FollowupFinding] = Field( + default_factory=list, description="Issues found in contributor comments" + ) + verdict: Literal[ + "READY_TO_MERGE", "MERGE_WITH_CHANGES", "NEEDS_REVISION", "BLOCKED" + ] = Field(description="Overall merge verdict") + verdict_reasoning: str = Field(description="Explanation for the verdict") + + +# ============================================================================= +# Initial Review Responses (Multi-Pass) +# ============================================================================= + + +class QuickScanResult(BaseModel): + """Result from the quick scan pass.""" + + purpose: str = Field(description="Brief description of what the PR claims to do") + actual_changes: str = Field( + description="Brief description of what the code actually does" + ) + purpose_match: bool = Field( + description="Whether actual changes match the claimed purpose" + ) + purpose_match_note: str | None = Field( + None, description="Explanation if purpose doesn't match actual changes" + ) + risk_areas: list[str] = Field( + default_factory=list, description="Areas needing careful review" + ) + red_flags: list[str] = Field( + default_factory=list, description="Obvious issues or concerns" + ) + requires_deep_verification: bool = Field( + description="Whether deep verification is needed" + ) + complexity: Literal["low", "medium", "high"] = Field(description="PR complexity") + + +class SecurityPassResult(BaseModel): + """Result from the security pass - array of security findings.""" + + findings: list[SecurityFinding] = Field( + default_factory=list, description="Security vulnerabilities found" + ) + + +class QualityPassResult(BaseModel): + """Result from the quality pass - array of quality findings.""" + + findings: list[QualityFinding] = Field( + default_factory=list, description="Quality and redundancy issues found" + ) + + +class DeepAnalysisResult(BaseModel): + """Result from the deep analysis pass.""" + + findings: list[DeepAnalysisFinding] = Field( + default_factory=list, + description="Deep analysis findings with verification info", + ) + + +class StructuralPassResult(BaseModel): + """Result from the structural pass.""" + + issues: list[StructuralIssue] = Field( + default_factory=list, description="Structural issues found" + ) + verdict: Literal[ + "READY_TO_MERGE", "MERGE_WITH_CHANGES", "NEEDS_REVISION", "BLOCKED" + ] = Field(description="Structural verdict") + verdict_reasoning: str = Field(description="Explanation for the verdict") + + +class AICommentTriageResult(BaseModel): + """Result from AI comment triage pass.""" + + triages: list[AICommentTriage] = Field( + default_factory=list, description="Triage results for each AI comment" + ) + + +# ============================================================================= +# Issue Triage Response +# ============================================================================= + + +class IssueTriageResponse(BaseModel): + """Response for issue triage.""" + + category: Literal[ + "bug", + "feature", + "documentation", + "question", + "duplicate", + "spam", + "feature_creep", + ] = Field(description="Issue category") + confidence: float = Field( + ge=0.0, le=1.0, description="Confidence in the categorization (0.0-1.0)" + ) + priority: Literal["high", "medium", "low"] = Field(description="Issue priority") + labels_to_add: list[str] = Field( + default_factory=list, description="Labels to add to the issue" + ) + labels_to_remove: list[str] = Field( + default_factory=list, description="Labels to remove from the issue" + ) + is_duplicate: bool = Field(False, description="Whether this is a duplicate issue") + duplicate_of: int | None = Field( + None, description="Issue number this duplicates (if duplicate)" + ) + is_spam: bool = Field(False, description="Whether this is spam") + is_feature_creep: bool = Field( + False, description="Whether this bundles multiple unrelated features" + ) + suggested_breakdown: list[str] = Field( + default_factory=list, + description="Suggested breakdown if feature creep detected", + ) + comment: str | None = Field(None, description="Optional bot comment to post") + + +# ============================================================================= +# Orchestrator Review Response +# ============================================================================= + + +class OrchestratorFinding(BaseModel): + """A finding from the orchestrator review.""" + + file: str = Field(description="File path where issue was found") + line: int = Field(0, description="Line number of the issue") + title: str = Field(description="Brief issue title") + description: str = Field(description="Detailed explanation of the issue") + category: Literal[ + "security", + "quality", + "style", + "docs", + "redundancy", + "verification_failed", + "pattern", + "performance", + "logic", + "test", + ] = Field(description="Issue category") + severity: Literal["critical", "high", "medium", "low"] = Field( + description="Issue severity level" + ) + suggestion: str | None = Field(None, description="How to fix this issue") + confidence: float = Field( + 0.85, + ge=0.0, + le=1.0, + description="Confidence (0.0-1.0 or 0-100, normalized to 0.0-1.0)", + ) + + @field_validator("confidence", mode="before") + @classmethod + def normalize_confidence(cls, v: int | float) -> float: + """Normalize confidence to 0.0-1.0 range (accepts 0-100 or 0.0-1.0).""" + if v > 1: + return v / 100.0 + return float(v) + + +class OrchestratorReviewResponse(BaseModel): + """Complete response schema for orchestrator PR review.""" + + verdict: Literal[ + "READY_TO_MERGE", "MERGE_WITH_CHANGES", "NEEDS_REVISION", "BLOCKED" + ] = Field(description="Overall merge verdict") + verdict_reasoning: str = Field(description="Explanation for the verdict") + findings: list[OrchestratorFinding] = Field( + default_factory=list, description="Issues found during review" + ) + summary: str = Field(description="Brief summary of the review") diff --git a/apps/frontend/src/main/ipc-handlers/github/investigation-handlers.ts b/apps/frontend/src/main/ipc-handlers/github/investigation-handlers.ts index b5c39e65..4f5a36d4 100644 --- a/apps/frontend/src/main/ipc-handlers/github/investigation-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/github/investigation-handlers.ts @@ -110,7 +110,8 @@ export function registerInvestigateIssue( ) as GitHubAPIComment[]; // Filter comments based on selection (if provided) - const comments = selectedCommentIds && selectedCommentIds.length > 0 + // Use Array.isArray to handle empty array case (all comments deselected) + const comments = Array.isArray(selectedCommentIds) ? allComments.filter(c => selectedCommentIds.includes(c.id)) : allComments; diff --git a/apps/frontend/src/main/ipc-handlers/github/utils/subprocess-runner.ts b/apps/frontend/src/main/ipc-handlers/github/utils/subprocess-runner.ts index 6faae462..fd1b9e6b 100644 --- a/apps/frontend/src/main/ipc-handlers/github/utils/subprocess-runner.ts +++ b/apps/frontend/src/main/ipc-handlers/github/utils/subprocess-runner.ts @@ -55,7 +55,11 @@ export function runPythonSubprocess( // Don't set PYTHONPATH - let runner.py manage its own import paths // Setting PYTHONPATH can interfere with runner.py's sys.path manipulation // Filter environment variables to only include necessary ones (prevent leaking secrets) - const safeEnvVars = ['PATH', 'HOME', 'USER', 'SHELL', 'LANG', 'LC_ALL', 'TERM', 'TMPDIR', 'TMP', 'TEMP']; + // Note: DEBUG is included for PR review debugging (shows LLM thinking blocks). + // This is safe because: (1) user must explicitly enable via npm run dev:debug, + // (2) it only enables our internal debug logging, not third-party framework debugging, + // (3) no sensitive values are logged - only LLM reasoning and response text. + const safeEnvVars = ['PATH', 'HOME', 'USER', 'SHELL', 'LANG', 'LC_ALL', 'TERM', 'TMPDIR', 'TMP', 'TEMP', 'DEBUG']; const filteredEnv: Record = {}; for (const key of safeEnvVars) { if (process.env[key]) { diff --git a/auto-claude/DOCKER_NATIVE_DESIGN.md b/shared_docs/DOCKER_NATIVE_DESIGN.md similarity index 100% rename from auto-claude/DOCKER_NATIVE_DESIGN.md rename to shared_docs/DOCKER_NATIVE_DESIGN.md diff --git a/auto-claude/PROMPT_INJECTION_DEFENSE.md b/shared_docs/PROMPT_INJECTION_DEFENSE.md similarity index 100% rename from auto-claude/PROMPT_INJECTION_DEFENSE.md rename to shared_docs/PROMPT_INJECTION_DEFENSE.md diff --git a/tests/test_sdk_structured_output.py b/tests/test_sdk_structured_output.py new file mode 100644 index 00000000..4d375fda --- /dev/null +++ b/tests/test_sdk_structured_output.py @@ -0,0 +1,173 @@ +""" +Test Claude Agent SDK Structured Output functionality. + +This test verifies how structured outputs work with the SDK. +""" + +import asyncio +import os +import sys +from pathlib import Path +from pprint import pprint + +# Add backend to path +sys.path.insert(0, str(Path(__file__).parent.parent / "apps" / "backend")) + +# Add pydantic_models path +_pydantic_models_path = ( + Path(__file__).parent.parent + / "apps" + / "backend" + / "runners" + / "github" + / "services" +) +sys.path.insert(0, str(_pydantic_models_path)) + +from pydantic import BaseModel, Field +from typing import Literal + + +# Simple test model +class SimpleReviewResponse(BaseModel): + """A simple review response for testing.""" + + verdict: Literal["PASS", "FAIL"] = Field(description="Review verdict") + reason: str = Field(description="Reason for the verdict") + score: int = Field(ge=0, le=100, description="Score from 0-100") + + +async def test_structured_output(): + """Test the SDK's structured output functionality.""" + + # OAuth token must be set in environment (CLAUDE_CODE_OAUTH_TOKEN) + if not os.environ.get("CLAUDE_CODE_OAUTH_TOKEN"): + print("ERROR: CLAUDE_CODE_OAUTH_TOKEN environment variable not set") + return + + from claude_agent_sdk import query, ClaudeAgentOptions + + # Generate JSON schema from Pydantic model + schema = SimpleReviewResponse.model_json_schema() + print("=== Schema ===") + pprint(schema) + print() + + prompt = """ +Review this code and provide your assessment: + +```python +def add(a, b): + return a + b +``` + +Provide a verdict (PASS or FAIL), reason, and score. +""" + + print("=== Running query with output_format ===") + print(f"Prompt: {prompt[:100]}...") + print() + + message_count = 0 + async for message in query( + prompt=prompt, + options=ClaudeAgentOptions( + model="claude-sonnet-4-5-20250929", + system_prompt="You are a code reviewer. Provide structured feedback.", + allowed_tools=[], + max_turns=2, # Need 2 turns for structured output tool call + output_format={ + "type": "json_schema", + "schema": schema, + }, + ), + ): + message_count += 1 + msg_type = type(message).__name__ + print(f"\n=== Message {message_count}: {msg_type} ===") + + # Print all non-private attributes + for attr in dir(message): + if not attr.startswith("_"): + try: + val = getattr(message, attr) + if not callable(val): + # Truncate long values + val_str = str(val) + if len(val_str) > 500: + val_str = val_str[:500] + "..." + print(f" {attr}: {val_str}") + except Exception as e: + print(f" {attr}: ") + + # Check for StructuredOutput tool use in AssistantMessage + if msg_type == "AssistantMessage": + content = getattr(message, "content", []) + for block in content: + block_type = type(block).__name__ + if block_type == "ToolUseBlock": + tool_name = getattr(block, "name", "") + if tool_name == "StructuredOutput": + structured_data = getattr(block, "input", None) + print(f"\n 🎯 Found StructuredOutput tool use!") + print(f" Data: {structured_data}") + if structured_data: + try: + validated = SimpleReviewResponse.model_validate(structured_data) + print(f"\n ✅ Successfully validated StructuredOutput!") + print(f" verdict: {validated.verdict}") + print(f" reason: {validated.reason}") + print(f" score: {validated.score}") + except Exception as e: + print(f"\n ❌ Failed to validate: {e}") + + # Special handling for ResultMessage + if msg_type == "ResultMessage": + print("\n --- ResultMessage Details ---") + + # Check structured_output + so = getattr(message, "structured_output", None) + print(f" structured_output value: {so}") + print(f" structured_output type: {type(so)}") + + # Check result + result = getattr(message, "result", None) + print(f" result value: {result}") + print(f" result type: {type(result)}") + + # If result is a string, try to parse as JSON + if isinstance(result, str): + import json + try: + parsed = json.loads(result) + print(f" result parsed as JSON: {parsed}") + except: + print(f" result is not JSON") + + # Try to validate with Pydantic if we got data + if so: + try: + validated = SimpleReviewResponse.model_validate(so) + print(f"\n ✅ Successfully validated structured_output!") + print(f" verdict: {validated.verdict}") + print(f" reason: {validated.reason}") + print(f" score: {validated.score}") + except Exception as e: + print(f"\n ❌ Failed to validate structured_output: {e}") + + if result and isinstance(result, (dict, str)): + try: + data = result if isinstance(result, dict) else json.loads(result) + validated = SimpleReviewResponse.model_validate(data) + print(f"\n ✅ Successfully validated result as structured output!") + print(f" verdict: {validated.verdict}") + print(f" reason: {validated.reason}") + print(f" score: {validated.score}") + except Exception as e: + print(f"\n ❌ Failed to validate result: {e}") + + print(f"\n=== Total messages: {message_count} ===") + + +if __name__ == "__main__": + asyncio.run(test_structured_output()) diff --git a/tests/test_structured_outputs.py b/tests/test_structured_outputs.py new file mode 100644 index 00000000..dc5f34a5 --- /dev/null +++ b/tests/test_structured_outputs.py @@ -0,0 +1,459 @@ +""" +Tests for Pydantic Structured Output Models +============================================ + +Tests the Pydantic models used for Claude Agent SDK structured outputs +in GitHub PR reviews. +""" + +import sys +from pathlib import Path + +import pytest +from pydantic import ValidationError + +# Direct import of pydantic_models to avoid runners package chain +# Path is set up by conftest.py +_pydantic_models_path = ( + Path(__file__).parent.parent + / "apps" + / "backend" + / "runners" + / "github" + / "services" +) +sys.path.insert(0, str(_pydantic_models_path)) + +from pydantic_models import ( + # Follow-up review models + FindingResolution, + FollowupFinding, + FollowupReviewResponse, + # Orchestrator review models + OrchestratorFinding, + OrchestratorReviewResponse, + # Initial review models + QuickScanResult, + SecurityFinding, + QualityFinding, + DeepAnalysisFinding, + StructuralIssue, + AICommentTriage, +) + + +class TestFindingResolution: + """Tests for FindingResolution model.""" + + def test_valid_resolution_resolved(self): + """Test valid resolved finding.""" + data = { + "finding_id": "prev-1", + "status": "resolved", + "resolution_notes": "Fixed in commit abc123", + } + result = FindingResolution.model_validate(data) + assert result.finding_id == "prev-1" + assert result.status == "resolved" + assert result.resolution_notes == "Fixed in commit abc123" + + def test_valid_resolution_unresolved(self): + """Test valid unresolved finding.""" + data = { + "finding_id": "prev-2", + "status": "unresolved", + } + result = FindingResolution.model_validate(data) + assert result.status == "unresolved" + assert result.resolution_notes is None + + def test_invalid_status_rejected(self): + """Test that invalid status values are rejected.""" + data = { + "finding_id": "prev-1", + "status": "pending", # Invalid - not in Literal + } + with pytest.raises(ValidationError) as exc_info: + FindingResolution.model_validate(data) + assert "status" in str(exc_info.value) + + +class TestFollowupFinding: + """Tests for FollowupFinding model.""" + + def test_valid_finding(self): + """Test valid follow-up finding.""" + data = { + "id": "new-1", + "severity": "high", + "category": "security", + "title": "SQL Injection vulnerability", + "description": "User input not sanitized before query", + "file": "api/query.py", + "line": 42, + "suggested_fix": "Use parameterized queries", + "fixable": True, + } + result = FollowupFinding.model_validate(data) + assert result.id == "new-1" + assert result.severity == "high" + assert result.category == "security" + assert result.line == 42 + assert result.fixable is True + + def test_minimal_finding(self): + """Test finding with only required fields.""" + data = { + "id": "new-2", + "severity": "low", + "category": "docs", + "title": "Missing docstring", + "description": "Function lacks documentation", + "file": "utils.py", + } + result = FollowupFinding.model_validate(data) + assert result.line == 0 # Default + assert result.suggested_fix is None + assert result.fixable is False + + def test_invalid_severity_rejected(self): + """Test that invalid severity is rejected.""" + data = { + "id": "new-1", + "severity": "extreme", # Invalid + "category": "security", + "title": "Test", + "description": "Test", + "file": "test.py", + } + with pytest.raises(ValidationError) as exc_info: + FollowupFinding.model_validate(data) + assert "severity" in str(exc_info.value) + + def test_invalid_category_rejected(self): + """Test that invalid category is rejected.""" + data = { + "id": "new-1", + "severity": "high", + "category": "unknown_category", # Invalid + "title": "Test", + "description": "Test", + "file": "test.py", + } + with pytest.raises(ValidationError) as exc_info: + FollowupFinding.model_validate(data) + assert "category" in str(exc_info.value) + + +class TestFollowupReviewResponse: + """Tests for FollowupReviewResponse model.""" + + def test_valid_complete_response(self): + """Test valid complete follow-up review response.""" + data = { + "finding_resolutions": [ + {"finding_id": "prev-1", "status": "resolved", "resolution_notes": "Fixed"} + ], + "new_findings": [ + { + "id": "new-1", + "severity": "medium", + "category": "quality", + "title": "Code smell", + "description": "Complex method", + "file": "service.py", + "line": 100, + } + ], + "comment_findings": [], + "verdict": "MERGE_WITH_CHANGES", + "verdict_reasoning": "Minor issues found, safe to merge after review", + } + result = FollowupReviewResponse.model_validate(data) + assert result.verdict == "MERGE_WITH_CHANGES" + assert len(result.finding_resolutions) == 1 + assert len(result.new_findings) == 1 + assert len(result.comment_findings) == 0 + + def test_empty_findings_lists(self): + """Test response with empty findings lists.""" + data = { + "finding_resolutions": [], + "new_findings": [], + "comment_findings": [], + "verdict": "READY_TO_MERGE", + "verdict_reasoning": "No issues found", + } + result = FollowupReviewResponse.model_validate(data) + assert result.verdict == "READY_TO_MERGE" + + def test_invalid_verdict_rejected(self): + """Test that invalid verdict is rejected.""" + data = { + "finding_resolutions": [], + "new_findings": [], + "comment_findings": [], + "verdict": "APPROVE", # Invalid + "verdict_reasoning": "Test", + } + with pytest.raises(ValidationError) as exc_info: + FollowupReviewResponse.model_validate(data) + assert "verdict" in str(exc_info.value) + + def test_all_verdict_values(self): + """Test all valid verdict values.""" + for verdict in [ + "READY_TO_MERGE", + "MERGE_WITH_CHANGES", + "NEEDS_REVISION", + "BLOCKED", + ]: + data = { + "finding_resolutions": [], + "new_findings": [], + "comment_findings": [], + "verdict": verdict, + "verdict_reasoning": f"Testing {verdict}", + } + result = FollowupReviewResponse.model_validate(data) + assert result.verdict == verdict + + +class TestOrchestratorFinding: + """Tests for OrchestratorFinding model.""" + + def test_valid_finding(self): + """Test valid orchestrator finding.""" + data = { + "file": "src/api.py", + "line": 25, + "title": "Missing error handling", + "description": "API endpoint lacks try-catch block", + "category": "quality", + "severity": "medium", + "suggestion": "Add error handling with proper logging", + "confidence": 90, + } + result = OrchestratorFinding.model_validate(data) + assert result.file == "src/api.py" + assert result.confidence == 0.9 # 90 normalized to 0.9 + + def test_confidence_bounds(self): + """Test confidence bounds (accepts 0-100 or 0.0-1.0, normalized to 0.0-1.0).""" + # Valid min + data = { + "file": "test.py", + "title": "Test", + "description": "Test", + "category": "quality", + "severity": "low", + "confidence": 0, + } + result = OrchestratorFinding.model_validate(data) + assert result.confidence == 0 # 0 stays as 0 + + # Valid max (100% normalized to 1.0) + data["confidence"] = 100 + result = OrchestratorFinding.model_validate(data) + assert result.confidence == 1.0 # 100 normalized to 1.0 + + # Invalid: over 100 (would normalize to >1.0) + data["confidence"] = 101 + with pytest.raises(ValidationError): + OrchestratorFinding.model_validate(data) + + # Invalid: negative + data["confidence"] = -1 + with pytest.raises(ValidationError): + OrchestratorFinding.model_validate(data) + + +class TestOrchestratorReviewResponse: + """Tests for OrchestratorReviewResponse model.""" + + def test_valid_response(self): + """Test valid orchestrator review response.""" + data = { + "verdict": "NEEDS_REVISION", + "verdict_reasoning": "Critical security issue found", + "findings": [ + { + "file": "auth.py", + "line": 10, + "title": "Hardcoded secret", + "description": "API key exposed in source", + "category": "security", + "severity": "critical", + "confidence": 95, + } + ], + "summary": "Found 1 critical security issue", + } + result = OrchestratorReviewResponse.model_validate(data) + assert result.verdict == "NEEDS_REVISION" + assert len(result.findings) == 1 + assert result.findings[0].severity == "critical" + + def test_empty_findings(self): + """Test response with no findings.""" + data = { + "verdict": "READY_TO_MERGE", + "verdict_reasoning": "All checks passed", + "findings": [], + "summary": "Clean PR, ready for merge", + } + result = OrchestratorReviewResponse.model_validate(data) + assert len(result.findings) == 0 + + +class TestQuickScanResult: + """Tests for QuickScanResult model.""" + + def test_valid_quick_scan(self): + """Test valid quick scan result.""" + data = { + "purpose": "Add user authentication", + "actual_changes": "Implements OAuth login flow", + "purpose_match": True, + "risk_areas": ["Security", "Session management"], + "red_flags": [], + "requires_deep_verification": True, + "complexity": "medium", + } + result = QuickScanResult.model_validate(data) + assert result.purpose_match is True + assert result.complexity == "medium" + assert len(result.risk_areas) == 2 + + def test_complexity_values(self): + """Test all valid complexity values.""" + for complexity in ["low", "medium", "high"]: + data = { + "purpose": "Test", + "actual_changes": "Test", + "purpose_match": True, + "requires_deep_verification": False, + "complexity": complexity, + } + result = QuickScanResult.model_validate(data) + assert result.complexity == complexity + + +class TestSchemaGeneration: + """Tests for JSON schema generation.""" + + def test_followup_schema_generation(self): + """Test that FollowupReviewResponse generates valid JSON schema.""" + schema = FollowupReviewResponse.model_json_schema() + + assert "properties" in schema + assert "verdict" in schema["properties"] + assert "verdict_reasoning" in schema["properties"] + assert "finding_resolutions" in schema["properties"] + assert "new_findings" in schema["properties"] + + # Check verdict enum values + verdict_schema = schema["properties"]["verdict"] + assert "enum" in verdict_schema or "$ref" in str(schema) + + def test_orchestrator_schema_generation(self): + """Test that OrchestratorReviewResponse generates valid JSON schema.""" + schema = OrchestratorReviewResponse.model_json_schema() + + assert "properties" in schema + assert "verdict" in schema["properties"] + assert "findings" in schema["properties"] + assert "summary" in schema["properties"] + + def test_schema_has_descriptions(self): + """Test that schema includes field descriptions for AI guidance.""" + schema = FollowupReviewResponse.model_json_schema() + + # Check that descriptions are included (helps AI understand the schema) + # The schema may have $defs for nested models + assert "properties" in schema or "$defs" in schema + + +class TestSecurityFinding: + """Tests for SecurityFinding model.""" + + def test_security_category_default(self): + """Test that SecurityFinding has security category by default.""" + data = { + "id": "sec-1", + "severity": "high", + "title": "XSS vulnerability", + "description": "Unescaped user input", + "file": "template.html", + "line": 50, + } + result = SecurityFinding.model_validate(data) + assert result.category == "security" + + +class TestDeepAnalysisFinding: + """Tests for DeepAnalysisFinding model.""" + + def test_confidence_float(self): + """Test confidence is a float between 0 and 1.""" + data = { + "id": "deep-1", + "severity": "medium", + "title": "Potential race condition", + "description": "Concurrent access without lock", + "file": "worker.py", + "line": 100, + "category": "logic", + "confidence": 0.75, + } + result = DeepAnalysisFinding.model_validate(data) + assert result.confidence == 0.75 + + def test_verification_note(self): + """Test verification note field.""" + data = { + "id": "deep-2", + "severity": "low", + "title": "Unverified assumption", + "description": "Could not verify behavior", + "file": "lib.py", + "category": "verification_failed", + "verification_note": "Unable to find test coverage", + } + result = DeepAnalysisFinding.model_validate(data) + assert result.verification_note == "Unable to find test coverage" + + +class TestAICommentTriage: + """Tests for AICommentTriage model.""" + + def test_valid_triage(self): + """Test valid AI comment triage.""" + data = { + "comment_id": 12345, + "tool_name": "CodeRabbit", + "verdict": "important", + "reasoning": "Valid security concern raised", + "response_comment": "Thank you, we will address this.", + } + result = AICommentTriage.model_validate(data) + assert result.comment_id == 12345 + assert result.verdict == "important" + + def test_all_verdict_values(self): + """Test all valid triage verdict values.""" + for verdict in [ + "critical", + "important", + "nice_to_have", + "trivial", + "false_positive", + ]: + data = { + "comment_id": 1, + "tool_name": "Test", + "verdict": verdict, + "reasoning": f"Testing {verdict}", + } + result = AICommentTriage.model_validate(data) + assert result.verdict == verdict