fix(github): improve PR review with structured outputs and fork support (#363)
* docs: add PR hygiene guidelines to CONTRIBUTING.md This update introduces a new section on PR hygiene, outlining best practices for rebasing, commit organization, and maintaining small PR sizes. It emphasizes the importance of keeping a clean commit history and provides commands for ensuring branches are up-to-date before requesting reviews. These guidelines aim to improve the overall quality and efficiency of pull requests in the project. Signed-off-by: AndyMik90 <andre@mikalsenutvikling.no> * fix(github-pr): use commit SHAs for PR context gathering and add debug logging Fixes GitHub PR review failing to retrieve file patches when PR branches aren't fetched locally (e.g., fork PRs, deleted branches). The context gatherer now fetches commit SHAs (headRefOid/baseRefOid) from GitHub API and uses them instead of branch names for git operations. Also adds comprehensive debug logging for the orchestrator reviewer: - Shows LLM thinking blocks and response streaming in DEBUG mode - Passes DEBUG env var through to Python subprocess - Adds status messages during long-running LLM calls Changes: - context_gatherer.py: Add _ensure_pr_refs_available() to fetch commits - orchestrator_reviewer.py: Add DEBUG_MODE logging for LLM interactions - subprocess-runner.ts: Pass DEBUG env var to Python subprocess - pydantic_models.py: Add structured output models for PR review 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: AndyMik90 <andre@mikalsenutvikling.no> * fix(github-pr): fix confidence conversion bugs in orchestrator reviewer Fixed 4 instances of broken confidence conversion logic: - Dead code where both ternary branches were identical (divided by 100) - Multiple data.get() calls with different defaults (85, 85, 0.85) Added _normalize_confidence() helper method that properly handles: - Percentage values (0-100): divides by 100 - Decimal values (0.0-1.0): uses as-is 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: AndyMik90 <andre@mikalsenutvikling.no> * fix(github-pr): address review findings and fix confidence normalization Address Auto Claude PR review findings: - Add Pydantic field_validator for confidence normalization (0-100 → 0.0-1.0) - Add path/ref validation helpers for command injection defense - Add fallback to text parsing when structured output fails - Sync category mapping between orchestrator and followup reviewer - Add security comment for DEBUG env var passthrough - Fix constraint from le=100.0 to le=1.0 for normalized confidence - Update tests to expect normalized confidence values 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: AndyMik90 <andre@mikalsenutvikling.no> * fix(github): extract structured output from SDK ToolUseBlock The Claude Agent SDK delivers structured outputs via a ToolUseBlock named 'StructuredOutput' in AssistantMessage.content, not in a structured_output attribute on the message. This was causing reviews to fall back to heuristic parsing instead of using validated JSON. Changes: - followup_reviewer: increased max_turns from 1 to 2 (structured output requires tool call + response), now extracts data from ToolUseBlock with name='StructuredOutput' - orchestrator_reviewer: added handling for StructuredOutput tool in both ToolUseBlock messages and AssistantMessage content - Added SDK structured output integration test 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: AndyMik90 <andre@mikalsenutvikling.no> * fix(github-pr): address Cursor review findings - Fix empty findings fallback logic: return None from _parse_structured_output on parsing failure instead of empty list, so clean PRs don't trigger unnecessary text parsing fallback - Handle _ensure_pr_refs_available return value: log warning if PR refs can't be fetched locally (will use GitHub API patches as fallback) - Add missing "docs" and "style" categories to OrchestratorFinding schema to match ReviewCategory enum and prevent validation failures 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: AndyMik90 <andre@mikalsenutvikling.no> * cleanup --------- Signed-off-by: AndyMik90 <andre@mikalsenutvikling.no> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -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!)
|
||||
|
||||
+37
-27
@@ -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))
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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")
|
||||
@@ -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;
|
||||
|
||||
|
||||
@@ -55,7 +55,11 @@ export function runPythonSubprocess<T = unknown>(
|
||||
// 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<string, string> = {};
|
||||
for (const key of safeEnvVars) {
|
||||
if (process.env[key]) {
|
||||
|
||||
@@ -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}: <error: {e}>")
|
||||
|
||||
# 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())
|
||||
@@ -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
|
||||
Reference in New Issue
Block a user