bfc232825b
* gsd update * docs: define v1 requirements with holistic PR understanding 29 requirements across 6 categories: - Holistic PR Understanding (5) - context synthesis and passing - Validation Pipeline (3) - finding-validator for all reviews - Schema Enforcement (5) - VerificationEvidence required - Prompt Improvements (6) - understand intent, evidence requirements - Code Simplification (6) - remove programmatic filters - Measurement (4) - 5 PRs to validate Key addition: Pass gathered context (related files, import graph) to specialists. Currently gathered but unused. * feat(01-01): add Phase 0 synthesis instruction to orchestrator prompt - Add 'Phase 0: Understand the PR Holistically' section before Phase 1 - Include PR UNDERSTANDING output format (intent, critical changes, risk areas, files to verify) - Add explicit gate: 'Only AFTER completing Phase 0, proceed to Phase 1' - Add 'Understand First' principle to Key Principles section Covers: CONTEXT-01, CONTEXT-05 * feat(01-01): add related files and import graph to orchestrator prompt - Add related files section categorizing tests vs dependencies/callers - Add import graph section showing what files import/are imported by changed files - Limit to 30 related files (15 tests, 15 deps) and 20 import entries - Include actionable guidance for using the context Covers: CONTEXT-02, CONTEXT-03 * feat(01-02): add investigation context to specialist agent descriptions - security-reviewer: check related files for affected callers, verify tests - quality-reviewer: check related files for pattern consistency - logic-reviewer: check callers/dependents for broken assumptions - codebase-fit-reviewer: use related files to understand existing patterns - finding-validator: check related files for missed mitigations - ai-triage-reviewer: unchanged (doesn't need related file guidance) CONTEXT-04: Specialists now know which files to investigate beyond the diff * feat(01-02): add specialist-specific delegation guidance to related files section - Updated header: "Pass relevant files to specialists when delegating" - Added per-specialist guidance for security, logic, quality, codebase-fit - Added example delegation showing how to include related files in task Orchestrator now knows HOW to pass investigation context to each specialist type * feat(02-01): add VerificationEvidence class and update finding models - Add VerificationEvidence class with required code_examined, line_range_examined, verification_method fields - Add required verification field to BaseFinding - Add required verification field to ParallelOrchestratorFinding - Add is_impact_finding boolean field to ParallelOrchestratorFinding (default False) - Add checked_for_handling_elsewhere boolean field to ParallelOrchestratorFinding (default False) - Mark old evidence field as DEPRECATED in both BaseFinding and ParallelOrchestratorFinding Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * test(02-01): add tests for schema enforcement and verification evidence - Add TestVerificationEvidence class with 5 tests for VerificationEvidence model - Add TestParallelOrchestratorFindingVerification class with 6 tests for verification requirement - Add TestVerificationSchemaGeneration class with 2 tests for JSON schema generation - Update existing TestSecurityFinding and TestDeepAnalysisFinding to include verification field - Import VerificationEvidence, ParallelOrchestratorFinding, BaseFinding in test imports Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * feat(03-02): add 'What the Diff Is For' section to orchestrator - Reframe diff as question to investigate, not document to nitpick - Add 3 questions to answer before delegation - Include 'Delegate with Context' guidance - Position after Phase 0, before Phase 1 * feat(03-01): add Understand Intent phase to all specialist prompts - Add Phase 1: Understand the PR Intent to security, logic, quality, codebase_fit agents - Force AI to understand PR purpose before searching for issues - Prevents flagging intentional design decisions as bugs Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * feat(03-02): enhance delegation guidance with context requirements - Add Context-Rich Delegation section with 3 requirements - Include PR intent summary, specific concerns, files of interest - Show anti-pattern vs good pattern comparison - Update example delegation with specific verification items * feat(03-01): add Evidence Requirements and Valid Outputs sections - Add Evidence Requirements section documenting VerificationEvidence schema - Document code_examined, line_range_examined, verification_method fields - Document is_impact_finding and checked_for_handling_elsewhere fields - Add Valid Outputs section allowing no-issues as valid output - Document invalid outputs (forced issues, theoretical edge cases) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * feat(03-01): update output format examples with verification object - Add verification object with code_examined, line_range_examined, verification_method - Add is_impact_finding and checked_for_handling_elsewhere fields - Use domain-appropriate verification_method values per agent - Security: direct_code_inspection for injection examples - Logic: direct_code_inspection for off-by-one and race conditions - Quality: direct_code_inspection + cross_file_trace for duplication - Codebase fit: cross_file_trace for reinvention, direct_code_inspection for naming Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * feat(04-01): add _verify_line_numbers() method - Pre-filter findings with invalid line numbers before AI validation - Cache file line counts to avoid re-reading same file - Reject findings where line > file length - Log each rejection with finding ID and reason - Conservative: allow findings if file read fails * feat(04-02): add hypothesis-validation structure to finding validator - Add "Hypothesis-Validation Structure (MANDATORY)" section with 4 steps - Define TRUE/FALSE conditions for hypothesis testing - Include worked example showing confirmed_valid conclusion path - Include counter-example showing dismissed_false_positive path - Reference structure from Investigation Process section * feat(04-01): add _validate_findings() method - Import FindingValidationResponse from pydantic_models - Create finding-validator agent client with pr_finding_validator type - Build validation prompt with findings JSON and changed files - Filter findings by validation_status: - confirmed_valid: keep with validation evidence - dismissed_false_positive: exclude from results - needs_human_review: keep with [NEEDS REVIEW] prefix - Fail-safe: return original findings on any error - Log validation statistics * feat(04-01): wire validation pipeline into review() method - Stage 1: Line verification after cross-validation (cheap pre-filter) - Stage 2: AI validation for findings that pass line check - Update programmatic filter loop to use validated_by_ai - Log validation statistics at each stage - Uses project_root (worktree or fallback) for file access * refactor(05-01): remove evidence filter and confidence routing from review() - Remove _validate_finding_evidence() call from loop - Remove _apply_confidence_routing() call - Simplify loop to only check scope - Replace routed_findings with direct validated_findings assignment * feat(05-02): remove false positive patterns from validator - Remove VAGUE_PATTERNS constant (10 patterns) - Remove GENERIC_PATTERNS constant (6 patterns) - Remove _is_false_positive() method (44 lines) - Remove _is_false_positive call from _is_valid() - Remove TestFalsePositiveDetection class (4 tests) - Update test_low_severity_higher_threshold to use actionability score REMOVE-04: VAGUE_PATTERNS, GENERIC_PATTERNS deleted REMOVE-05: _is_false_positive() method deleted * refactor(05-01): remove redundant functions and simplify scope check - Remove ConfidenceTier enum (no longer used) - Remove _validate_finding_evidence function (schema enforces evidence) - Remove _apply_confidence_routing method (validation is binary) - Remove 'from enum import Enum' import - Simplify _is_finding_in_scope to use schema field is_impact_finding instead of keyword detection * fix(pr-review): add Task tool to orchestrator configs for SDK subagents The pr_orchestrator_parallel and pr_followup_parallel agents need the Task tool in their tools list to invoke SDK subagents (security-reviewer, logic-reviewer, etc.). Without Task, the SDK cannot spawn subagents, resulting in "Agent type not found" errors. Also fixes test_integration_phase4.py to set is_impact_finding as an attribute rather than constructor arg, since PRReviewFinding doesn't have this field (it's on ParallelOrchestratorFinding Pydantic model). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(pr-review): add explicit Task tool invocation syntax for specialist agents The orchestrator was using the built-in general-purpose agent instead of our custom specialist agents (security-reviewer, logic-reviewer, etc.) because the prompt described agents but didn't show explicit Task tool invocation syntax. Changes: - Add "CRITICAL: How to Invoke Specialist Agents" section with exact subagent_type values in a reference table - Add Task tool invocation format with example syntax - Add example showing parallel invocation of multiple specialists - Add explicit "DO NOT USE" section warning against general-purpose - Update example delegation to use Task tool syntax instead of prose - Add example validation invocation for finding-validator This ensures Claude uses our custom specialists instead of defaulting to the built-in general-purpose agent. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * feat(pr-review): implement evidence-based validation and trigger-driven exploration Major enhancements to the PR review system: **Evidence-Based Validation:** - Shift from confidence-based to evidence-based finding validation - All findings now require VerificationEvidence with code_examined, line_range_examined - finding-validator validates ALL findings (CRITICAL through LOW) before output - Add dismissed_findings array for transparency - users see what was investigated **Trigger-Driven Exploration (6 Semantic Triggers):** - OUTPUT CONTRACT CHANGED - function returns different value/type/structure - INPUT CONTRACT CHANGED - parameters added/removed/reordered - BEHAVIORAL CONTRACT CHANGED - same I/O but different internal behavior - SIDE EFFECT CONTRACT CHANGED - observable effects added/removed - FAILURE CONTRACT CHANGED - error handling changed - NULL/UNDEFINED CONTRACT CHANGED - null handling changed Orchestrator detects triggers in Phase 1 and passes them to specialists with explicit "TRIGGER:", "EXPLORATION REQUIRED:", "Stop when:" instructions. **Implementation Changes:** - Add _PRDebugLogger for comprehensive agent communication logging - Add CI status integration to verdict logic (failing CI blocks merge) - Extract with_working_dir() to shared agent_utils.py module - Inject working directory into all subagent prompts - Bump SDK requirement to >=0.1.22 for custom subagent support **Frontend:** - Tighten AUTH_FAILURE_PATTERNS to avoid false positives on AI auth discussion - Update tests for new pattern requirements Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(pr-review): wait for both queued AND in_progress CI checks Previously, the CI wait logic only blocked on "in_progress" checks, but not "queued" checks. This meant if a CI check (like CodeRabbit) was queued but not yet running, the review would start immediately and report "CI is pending" - which would be stale by the time the contributor sees it. Now we wait for ALL checks to reach "completed" status before starting the review, ensuring the CI status in our review is accurate. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * chore: remove duplicate .planning entries from .gitignore Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * chore: remove docs/ from git tracking (already in .gitignore) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(pr-review): propagate is_impact_finding field to allow impact findings The is_impact_finding field was defined in ParallelOrchestratorFinding but never propagated to PRReviewFinding, causing ALL impact findings (findings about callers/affected files outside the PR's changed files) to be incorrectly filtered out as "not in scope". Changes: - Add is_impact_finding field to PRReviewFinding dataclass - Extract and pass is_impact_finding in _create_finding_from_structured() - Add to to_dict() and from_dict() for serialization This enables the trigger-driven exploration feature to actually work, allowing the review to report issues in files affected by contract changes. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(pr-review): add Task tool invocation syntax to followup orchestrator The follow-up review orchestrator was missing explicit Task tool invocation syntax and examples. The AI didn't know HOW to invoke the specialist agents (resolution-verifier, finding-validator, etc.), causing resolution checking to never happen. Added: - Exact agent names table (subagent_type values) - Task tool invocation format with examples - Complete follow-up review workflow with Task calls - DO NOT USE section (avoid general-purpose, Explore, Plan) - Decision matrix for when to invoke each agent - Explicit Task tool calls in Phase 2 workflow This matches the main orchestrator prompt which has extensive Task tool examples and works correctly. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(pr-review): propagate is_impact_finding in follow-up reviewer Applied the same is_impact_finding propagation fix to the follow-up reviewer that was already applied to the main orchestrator reviewer. Fixes: 1. Add is_impact_finding field to ParallelFollowupFinding Pydantic model 2. Propagate is_impact_finding when creating PRReviewFinding for new findings 3. Copy is_impact_finding from original finding for unresolved findings Without this fix, impact findings (about callers/affected files outside the PR's changed files) would be incorrectly filtered as "not in scope" during follow-up reviews. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * feat(auth): enhance keychain service integration with config directory support Added functionality to support profile-specific credentials by introducing a hash-based service name for macOS Keychain and updating Windows credential retrieval to utilize a provided config directory. This ensures that tokens are fetched from the correct profile-specific storage locations, improving credential management across different environments. Changes include: - New functions for calculating config directory hashes and generating keychain service names. - Updated `get_token_from_keychain` and related functions to accept an optional config directory argument. - Enhanced logging for better debugging when no token is found. This aligns the backend credential handling with the frontend's expectations for profile-specific storage. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
710 lines
26 KiB
Python
710 lines
26 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,
|
|
# Verification evidence models (Phase 2)
|
|
VerificationEvidence,
|
|
ParallelOrchestratorFinding,
|
|
BaseFinding,
|
|
)
|
|
|
|
|
|
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,
|
|
"verification": {
|
|
"code_examined": "query = 'SELECT * FROM users WHERE id=' + user_input",
|
|
"line_range_examined": [42, 42],
|
|
"verification_method": "direct_code_inspection",
|
|
},
|
|
}
|
|
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",
|
|
"verification": {
|
|
"code_examined": "def process_data(data):\n return data",
|
|
"line_range_examined": [1, 2],
|
|
"verification_method": "direct_code_inspection",
|
|
},
|
|
}
|
|
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,
|
|
"verification": {
|
|
"code_examined": "def process(self, data):\n # 50 lines of nested if statements",
|
|
"line_range_examined": [100, 150],
|
|
"verification_method": "direct_code_inspection",
|
|
},
|
|
}
|
|
],
|
|
"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 with evidence field."""
|
|
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",
|
|
"evidence": "def handle_request(req):\n result = db.query(req.id) # no try-catch",
|
|
"verification": {
|
|
"code_examined": "def handle_request(req):\n result = db.query(req.id) # no try-catch",
|
|
"line_range_examined": [25, 26],
|
|
"verification_method": "direct_code_inspection",
|
|
},
|
|
}
|
|
result = OrchestratorFinding.model_validate(data)
|
|
assert result.file == "src/api.py"
|
|
assert result.evidence is not None
|
|
assert "no try-catch" in result.evidence
|
|
|
|
def test_evidence_optional(self):
|
|
"""Test that evidence field is optional."""
|
|
data = {
|
|
"file": "test.py",
|
|
"title": "Test",
|
|
"description": "Test finding",
|
|
"category": "quality",
|
|
"severity": "low",
|
|
"verification": {
|
|
"code_examined": "def test():\n pass",
|
|
"line_range_examined": [1, 2],
|
|
"verification_method": "direct_code_inspection",
|
|
},
|
|
}
|
|
result = OrchestratorFinding.model_validate(data)
|
|
assert result.evidence is None
|
|
|
|
|
|
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",
|
|
"evidence": "API_KEY = 'sk-prod-12345abcdef'",
|
|
"verification": {
|
|
"code_examined": "API_KEY = 'sk-prod-12345abcdef'",
|
|
"line_range_examined": [10, 10],
|
|
"verification_method": "direct_code_inspection",
|
|
},
|
|
}
|
|
],
|
|
"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,
|
|
"verification": {
|
|
"code_examined": "<div>{{ user_input }}</div>",
|
|
"line_range_examined": [50, 50],
|
|
"verification_method": "direct_code_inspection",
|
|
},
|
|
}
|
|
result = SecurityFinding.model_validate(data)
|
|
assert result.category == "security"
|
|
|
|
|
|
class TestDeepAnalysisFinding:
|
|
"""Tests for DeepAnalysisFinding model."""
|
|
|
|
def test_evidence_field(self):
|
|
"""Test evidence field for proof of issue."""
|
|
data = {
|
|
"id": "deep-1",
|
|
"severity": "medium",
|
|
"title": "Potential race condition",
|
|
"description": "Concurrent access without lock",
|
|
"file": "worker.py",
|
|
"line": 100,
|
|
"category": "logic",
|
|
"evidence": "shared_state += 1 # no lock protection",
|
|
"verification": {
|
|
"code_examined": "shared_state += 1 # no lock protection",
|
|
"line_range_examined": [100, 100],
|
|
"verification_method": "direct_code_inspection",
|
|
},
|
|
}
|
|
result = DeepAnalysisFinding.model_validate(data)
|
|
assert result.evidence == "shared_state += 1 # no lock protection"
|
|
|
|
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",
|
|
"verification": {
|
|
"code_examined": "def some_function():\n return process_data()",
|
|
"line_range_examined": [1, 2],
|
|
"verification_method": "cross_file_trace",
|
|
},
|
|
}
|
|
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
|
|
|
|
|
|
# =============================================================================
|
|
# Phase 2: Schema Enforcement Tests
|
|
# =============================================================================
|
|
|
|
|
|
class TestVerificationEvidence:
|
|
"""Tests for VerificationEvidence model."""
|
|
|
|
def test_valid_verification(self):
|
|
"""Test valid verification evidence."""
|
|
data = {
|
|
"code_examined": "def process_input(user_input):\n return eval(user_input)",
|
|
"line_range_examined": [10, 11],
|
|
"verification_method": "direct_code_inspection",
|
|
}
|
|
result = VerificationEvidence.model_validate(data)
|
|
assert "eval" in result.code_examined
|
|
assert result.line_range_examined == [10, 11]
|
|
assert result.verification_method == "direct_code_inspection"
|
|
|
|
def test_empty_code_examined_rejected(self):
|
|
"""Test that empty code_examined is rejected."""
|
|
data = {
|
|
"code_examined": "",
|
|
"line_range_examined": [1, 5],
|
|
"verification_method": "direct_code_inspection",
|
|
}
|
|
with pytest.raises(ValidationError) as exc_info:
|
|
VerificationEvidence.model_validate(data)
|
|
assert "code_examined" in str(exc_info.value)
|
|
|
|
def test_invalid_line_range_rejected(self):
|
|
"""Test that invalid line ranges are rejected."""
|
|
data = {
|
|
"code_examined": "some code",
|
|
"line_range_examined": [1], # Should have exactly 2 elements
|
|
"verification_method": "direct_code_inspection",
|
|
}
|
|
with pytest.raises(ValidationError) as exc_info:
|
|
VerificationEvidence.model_validate(data)
|
|
assert "line_range_examined" in str(exc_info.value)
|
|
|
|
def test_invalid_verification_method_rejected(self):
|
|
"""Test that invalid verification method is rejected."""
|
|
data = {
|
|
"code_examined": "some code",
|
|
"line_range_examined": [1, 5],
|
|
"verification_method": "guessed", # Invalid method
|
|
}
|
|
with pytest.raises(ValidationError) as exc_info:
|
|
VerificationEvidence.model_validate(data)
|
|
assert "verification_method" in str(exc_info.value)
|
|
|
|
def test_all_verification_methods(self):
|
|
"""Test all valid verification methods."""
|
|
methods = [
|
|
"direct_code_inspection",
|
|
"cross_file_trace",
|
|
"test_verification",
|
|
"dependency_analysis",
|
|
]
|
|
for method in methods:
|
|
data = {
|
|
"code_examined": "code",
|
|
"line_range_examined": [1, 5],
|
|
"verification_method": method,
|
|
}
|
|
result = VerificationEvidence.model_validate(data)
|
|
assert result.verification_method == method
|
|
|
|
|
|
class TestParallelOrchestratorFindingVerification:
|
|
"""Tests for verification field requirement on ParallelOrchestratorFinding."""
|
|
|
|
def test_missing_verification_rejected(self):
|
|
"""Test that findings without verification are rejected."""
|
|
data = {
|
|
"id": "test-1",
|
|
"file": "test.py",
|
|
"line": 10,
|
|
"title": "Test finding",
|
|
"description": "A test finding without verification",
|
|
"category": "quality",
|
|
"severity": "medium",
|
|
# No verification field - should fail
|
|
}
|
|
with pytest.raises(ValidationError) as exc_info:
|
|
ParallelOrchestratorFinding.model_validate(data)
|
|
assert "verification" in str(exc_info.value)
|
|
|
|
def test_valid_finding_with_verification(self):
|
|
"""Test valid finding with verification evidence."""
|
|
data = {
|
|
"id": "test-1",
|
|
"file": "test.py",
|
|
"line": 10,
|
|
"title": "SQL Injection vulnerability",
|
|
"description": "User input passed directly to query",
|
|
"category": "security",
|
|
"severity": "critical",
|
|
"verification": {
|
|
"code_examined": "cursor.execute(f'SELECT * FROM users WHERE id={user_id}')",
|
|
"line_range_examined": [10, 10],
|
|
"verification_method": "direct_code_inspection",
|
|
},
|
|
}
|
|
result = ParallelOrchestratorFinding.model_validate(data)
|
|
assert result.verification.code_examined is not None
|
|
assert result.verification.verification_method == "direct_code_inspection"
|
|
|
|
def test_is_impact_finding_default_false(self):
|
|
"""Test is_impact_finding defaults to False."""
|
|
data = {
|
|
"id": "test-1",
|
|
"file": "test.py",
|
|
"line": 10,
|
|
"title": "Test",
|
|
"description": "Test",
|
|
"category": "quality",
|
|
"severity": "medium",
|
|
"verification": {
|
|
"code_examined": "code",
|
|
"line_range_examined": [10, 10],
|
|
"verification_method": "direct_code_inspection",
|
|
},
|
|
}
|
|
result = ParallelOrchestratorFinding.model_validate(data)
|
|
assert result.is_impact_finding is False
|
|
|
|
def test_is_impact_finding_true(self):
|
|
"""Test is_impact_finding can be set True."""
|
|
data = {
|
|
"id": "test-1",
|
|
"file": "caller.py",
|
|
"line": 50,
|
|
"title": "Breaking change affects caller",
|
|
"description": "This file calls the changed function and will break",
|
|
"category": "logic",
|
|
"severity": "high",
|
|
"is_impact_finding": True,
|
|
"verification": {
|
|
"code_examined": "result = changed_function(x)",
|
|
"line_range_examined": [50, 50],
|
|
"verification_method": "cross_file_trace",
|
|
},
|
|
}
|
|
result = ParallelOrchestratorFinding.model_validate(data)
|
|
assert result.is_impact_finding is True
|
|
|
|
def test_checked_for_handling_elsewhere_default_false(self):
|
|
"""Test checked_for_handling_elsewhere defaults to False."""
|
|
data = {
|
|
"id": "test-1",
|
|
"file": "test.py",
|
|
"line": 10,
|
|
"title": "Missing error handling",
|
|
"description": "No try-catch",
|
|
"category": "quality",
|
|
"severity": "medium",
|
|
"verification": {
|
|
"code_examined": "code",
|
|
"line_range_examined": [10, 10],
|
|
"verification_method": "direct_code_inspection",
|
|
},
|
|
}
|
|
result = ParallelOrchestratorFinding.model_validate(data)
|
|
assert result.checked_for_handling_elsewhere is False
|
|
|
|
def test_checked_for_handling_elsewhere_true(self):
|
|
"""Test checked_for_handling_elsewhere can be set True."""
|
|
data = {
|
|
"id": "test-1",
|
|
"file": "api.py",
|
|
"line": 25,
|
|
"title": "Missing error handling",
|
|
"description": "No try-catch around database call",
|
|
"category": "quality",
|
|
"severity": "medium",
|
|
"checked_for_handling_elsewhere": True,
|
|
"verification": {
|
|
"code_examined": "result = db.query(user_input)",
|
|
"line_range_examined": [25, 25],
|
|
"verification_method": "cross_file_trace",
|
|
},
|
|
}
|
|
result = ParallelOrchestratorFinding.model_validate(data)
|
|
assert result.checked_for_handling_elsewhere is True
|
|
|
|
|
|
class TestVerificationSchemaGeneration:
|
|
"""Tests for JSON schema generation with VerificationEvidence."""
|
|
|
|
def test_verification_in_parallel_orchestrator_schema(self):
|
|
"""Test that VerificationEvidence appears in schema."""
|
|
schema = ParallelOrchestratorFinding.model_json_schema()
|
|
|
|
# verification should be in properties
|
|
assert "verification" in schema["properties"]
|
|
|
|
# Check $defs includes VerificationEvidence
|
|
assert "$defs" in schema
|
|
assert "VerificationEvidence" in schema["$defs"]
|
|
|
|
# Check VerificationEvidence has correct fields
|
|
ve_schema = schema["$defs"]["VerificationEvidence"]
|
|
assert "code_examined" in ve_schema["properties"]
|
|
assert "line_range_examined" in ve_schema["properties"]
|
|
assert "verification_method" in ve_schema["properties"]
|
|
|
|
def test_new_boolean_fields_in_schema(self):
|
|
"""Test is_impact_finding and checked_for_handling_elsewhere in schema."""
|
|
schema = ParallelOrchestratorFinding.model_json_schema()
|
|
|
|
assert "is_impact_finding" in schema["properties"]
|
|
assert "checked_for_handling_elsewhere" in schema["properties"]
|