7751588e56
* 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>
460 lines
15 KiB
Python
460 lines
15 KiB
Python
"""
|
|
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
|