fix(pr-review): simplify structured output schema to reduce validation failures (#1787)
The ParallelFollowupResponse JSON schema was 10,743 chars with strict constraints, causing LLM structured output validation failures after long multi-agent sessions. Reduced to 4,561 chars (58% reduction) by removing unused fields and relaxing unnecessary constraints. - Remove unused fields: analysis_summary, commits_analyzed, files_changed, comment_analyses, agent_agreement, source_agent, related_to_previous, evidence (deprecated), end_line, and CommentAnalysis model - Relax constraints: remove min_length validators, make line_range optional, change verification_method from Literal to str with default - Update prompts to match simplified schema - Fix flaky test_allows_normal_commit by adding monkeypatch.chdir for git isolation during pre-commit hook execution Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Andy
@@ -271,9 +271,7 @@ Return one result per finding:
|
||||
"finding_id": "SEC-001",
|
||||
"validation_status": "confirmed_valid",
|
||||
"code_evidence": "const query = `SELECT * FROM users WHERE id = ${userId}`;",
|
||||
"line_range": [45, 45],
|
||||
"explanation": "SQL injection vulnerability confirmed. User input 'userId' is directly interpolated into the SQL query at line 45 without any sanitization. The query is executed via db.execute() on line 46.",
|
||||
"evidence_verified_in_file": true
|
||||
"explanation": "SQL injection vulnerability confirmed. User input 'userId' is directly interpolated into the SQL query at line 45 without any sanitization. The query is executed via db.execute() on line 46."
|
||||
}
|
||||
```
|
||||
|
||||
@@ -282,9 +280,7 @@ Return one result per finding:
|
||||
"finding_id": "QUAL-002",
|
||||
"validation_status": "dismissed_false_positive",
|
||||
"code_evidence": "function processInput(data: string): string {\n const sanitized = DOMPurify.sanitize(data);\n return sanitized;\n}",
|
||||
"line_range": [23, 26],
|
||||
"explanation": "The original finding claimed XSS vulnerability, but the code uses DOMPurify.sanitize() before output. The input is properly sanitized at line 24 before being returned. The code evidence proves the issue does NOT exist.",
|
||||
"evidence_verified_in_file": true
|
||||
"explanation": "The original finding claimed XSS vulnerability, but the code uses DOMPurify.sanitize() before output. The input is properly sanitized at line 24 before being returned."
|
||||
}
|
||||
```
|
||||
|
||||
@@ -293,9 +289,7 @@ Return one result per finding:
|
||||
"finding_id": "LOGIC-003",
|
||||
"validation_status": "needs_human_review",
|
||||
"code_evidence": "async function handleRequest(req) {\n // Complex async logic...\n}",
|
||||
"line_range": [100, 150],
|
||||
"explanation": "The original finding claims a race condition, but verifying this requires understanding the runtime behavior and concurrency model. The static code doesn't provide definitive evidence either way.",
|
||||
"evidence_verified_in_file": true
|
||||
"explanation": "The original finding claims a race condition, but verifying this requires understanding the runtime behavior and concurrency model. The static code doesn't provide definitive evidence either way."
|
||||
}
|
||||
```
|
||||
|
||||
@@ -304,9 +298,7 @@ Return one result per finding:
|
||||
"finding_id": "HALLUC-004",
|
||||
"validation_status": "dismissed_false_positive",
|
||||
"code_evidence": "// Line 710 does not exist - file only has 600 lines",
|
||||
"line_range": [600, 600],
|
||||
"explanation": "The original finding claimed an issue at line 710, but the file only has 600 lines. This is a hallucinated finding - the code doesn't exist.",
|
||||
"evidence_verified_in_file": false
|
||||
"explanation": "The original finding claimed an issue at line 710, but the file only has 600 lines. This is a hallucinated finding - the code doesn't exist."
|
||||
}
|
||||
```
|
||||
|
||||
@@ -324,7 +316,7 @@ Validation is binary based on what the code evidence shows:
|
||||
**Decision rules:**
|
||||
- If `code_evidence` contains problematic code → `confirmed_valid`
|
||||
- If `code_evidence` proves issue doesn't exist → `dismissed_false_positive`
|
||||
- If `evidence_verified_in_file` is false → `dismissed_false_positive` (hallucinated finding)
|
||||
- If the code/line doesn't exist → `dismissed_false_positive` (hallucinated finding)
|
||||
- If you can't determine from the code → `needs_human_review`
|
||||
|
||||
## Common False Positive Patterns
|
||||
@@ -403,7 +395,7 @@ CROSS-FILE CHECK:
|
||||
5. **When evidence is inconclusive, escalate** - Use `needs_human_review` rather than guessing
|
||||
6. **Look for mitigations** - Check surrounding code for sanitization/validation
|
||||
7. **Check the full context** - Read ±20 lines, not just the flagged line
|
||||
8. **Verify code exists** - Set `evidence_verified_in_file` to false if the code/line doesn't exist
|
||||
8. **Verify code exists** - Dismiss as false positive if the code/line doesn't exist
|
||||
9. **SEARCH BEFORE CLAIMING ABSENCE** - If you claim something doesn't exist (no helper, no validation, no error handling), you MUST show the search you performed:
|
||||
- Use Grep to search for the pattern
|
||||
- Include the search command in your explanation
|
||||
@@ -414,5 +406,5 @@ CROSS-FILE CHECK:
|
||||
- **Trusting the original finding blindly** - Always verify with actual code
|
||||
- **Dismissing without reading code** - Must provide code_evidence that proves your point
|
||||
- **Vague explanations** - Be specific about what the code shows and why it proves/disproves the issue
|
||||
- **Missing line numbers** - Always include line_range
|
||||
- **Vague evidence** - Always include actual code snippets
|
||||
- **Speculative conclusions** - Only conclude what the code evidence actually proves
|
||||
|
||||
@@ -303,35 +303,24 @@ Provide your synthesis as a structured response matching the ParallelFollowupRes
|
||||
|
||||
```json
|
||||
{
|
||||
"analysis_summary": "Brief summary of what was analyzed",
|
||||
"agents_invoked": ["resolution-verifier", "finding-validator", "new-code-reviewer"],
|
||||
"commits_analyzed": 5,
|
||||
"files_changed": 12,
|
||||
"resolution_verifications": [...],
|
||||
"finding_validations": [
|
||||
{
|
||||
"finding_id": "SEC-001",
|
||||
"validation_status": "confirmed_valid",
|
||||
"code_evidence": "const query = `SELECT * FROM users WHERE id = ${userId}`;",
|
||||
"line_range": [45, 45],
|
||||
"explanation": "SQL injection is present - user input is concatenated directly into query"
|
||||
},
|
||||
{
|
||||
"finding_id": "QUAL-002",
|
||||
"validation_status": "dismissed_false_positive",
|
||||
"code_evidence": "const sanitized = DOMPurify.sanitize(data);",
|
||||
"line_range": [23, 26],
|
||||
"explanation": "Original finding claimed XSS but code uses DOMPurify for sanitization"
|
||||
}
|
||||
],
|
||||
"new_findings": [...],
|
||||
"comment_analyses": [...],
|
||||
"comment_findings": [...],
|
||||
"agent_agreement": {
|
||||
"agreed_findings": [],
|
||||
"conflicting_findings": [],
|
||||
"resolution_notes": null
|
||||
},
|
||||
"verdict": "READY_TO_MERGE",
|
||||
"verdict_reasoning": "2 findings resolved, 1 dismissed as false positive, 1 confirmed valid but LOW severity..."
|
||||
}
|
||||
|
||||
@@ -34,41 +34,18 @@ from pydantic import BaseModel, Field
|
||||
|
||||
|
||||
class VerificationEvidence(BaseModel):
|
||||
"""Evidence that a finding was verified against actual code.
|
||||
|
||||
All fields are required - schema enforcement guarantees evidence exists.
|
||||
This shifts quality control from programmatic filters to schema enforcement.
|
||||
"""
|
||||
"""Evidence that a finding was verified against actual code."""
|
||||
|
||||
code_examined: str = Field(
|
||||
min_length=1,
|
||||
description=(
|
||||
"REQUIRED: Exact code snippet that was examined. "
|
||||
"Must be actual code from the file, not a description of code. "
|
||||
"Copy-paste the relevant lines directly."
|
||||
),
|
||||
description="Code snippet that was examined to verify the finding",
|
||||
)
|
||||
line_range_examined: list[int] = Field(
|
||||
min_length=2,
|
||||
max_length=2,
|
||||
description=(
|
||||
"Start and end line numbers [start, end] of the examined code. "
|
||||
"Must match the code in code_examined."
|
||||
),
|
||||
default_factory=list,
|
||||
description="Start and end line numbers [start, end] of the examined code",
|
||||
)
|
||||
verification_method: Literal[
|
||||
"direct_code_inspection",
|
||||
"cross_file_trace",
|
||||
"test_verification",
|
||||
"dependency_analysis",
|
||||
] = Field(
|
||||
description=(
|
||||
"How the issue was verified: "
|
||||
"direct_code_inspection = found issue directly in the code shown; "
|
||||
"cross_file_trace = traced through imports/calls to find the issue; "
|
||||
"test_verification = verified through examination of test code; "
|
||||
"dependency_analysis = verified through analyzing dependencies"
|
||||
)
|
||||
verification_method: str = Field(
|
||||
default="direct_code_inspection",
|
||||
description="How the issue was verified (e.g. direct_code_inspection, cross_file_trace, test_verification)",
|
||||
)
|
||||
|
||||
|
||||
@@ -630,22 +607,17 @@ class ResolutionVerification(BaseModel):
|
||||
Field(description="Resolution status after AI verification")
|
||||
)
|
||||
evidence: str = Field(
|
||||
min_length=1,
|
||||
description="Actual code snippet showing the resolution status. Required.",
|
||||
)
|
||||
resolution_notes: str | None = Field(
|
||||
None, description="Detailed notes on how the issue was addressed"
|
||||
description="Code snippet or explanation showing the resolution status",
|
||||
)
|
||||
|
||||
|
||||
class ParallelFollowupFinding(BaseModel):
|
||||
"""A finding from parallel follow-up review with source agent tracking."""
|
||||
"""A finding from parallel follow-up review."""
|
||||
|
||||
id: str = Field(description="Unique identifier for this finding")
|
||||
file: str = Field(description="File path where issue was found")
|
||||
line: int = Field(0, description="Line number of the issue")
|
||||
end_line: int | None = Field(None, description="End line for multi-line issues")
|
||||
title: str = Field(description="Brief issue title (max 80 chars)")
|
||||
title: str = Field(description="Brief issue title")
|
||||
description: str = Field(description="Detailed explanation of the issue")
|
||||
category: Literal[
|
||||
"security",
|
||||
@@ -659,99 +631,46 @@ class ParallelFollowupFinding(BaseModel):
|
||||
severity: Literal["critical", "high", "medium", "low"] = Field(
|
||||
description="Issue severity level"
|
||||
)
|
||||
evidence: str | None = Field(
|
||||
None,
|
||||
description="DEPRECATED: Use verification.code_examined instead. Will be removed in Phase 5.",
|
||||
)
|
||||
verification: VerificationEvidence = Field(
|
||||
description="Evidence that this finding was verified against actual code"
|
||||
)
|
||||
suggested_fix: str | None = Field(None, description="How to fix this issue")
|
||||
fixable: bool = Field(False, description="Whether this can be auto-fixed")
|
||||
source_agent: str = Field(
|
||||
description="Which agent reported this finding (resolution/newcode/comment)"
|
||||
)
|
||||
related_to_previous: str | None = Field(
|
||||
None, description="ID of related previous finding if this is a regression"
|
||||
)
|
||||
is_impact_finding: bool = Field(
|
||||
False,
|
||||
description=(
|
||||
"True if this finding is about impact on OTHER files (callers, dependents) "
|
||||
"outside the PR's changed files. Used by _is_finding_in_scope() to allow "
|
||||
"findings about related files that aren't directly in the PR diff."
|
||||
),
|
||||
description="True if this finding is about impact on OTHER files outside the PR diff",
|
||||
)
|
||||
|
||||
|
||||
class CommentAnalysis(BaseModel):
|
||||
"""Analysis of a contributor or AI comment."""
|
||||
|
||||
comment_id: str = Field(description="Identifier for the comment")
|
||||
author: str = Field(description="Comment author")
|
||||
is_ai_bot: bool = Field(description="Whether this is from an AI tool")
|
||||
requires_response: bool = Field(description="Whether this comment needs a response")
|
||||
sentiment: Literal["question", "concern", "suggestion", "praise", "neutral"] = (
|
||||
Field(description="Comment sentiment/type")
|
||||
)
|
||||
summary: str = Field(description="Brief summary of the comment")
|
||||
action_needed: str | None = Field(None, description="What action is needed if any")
|
||||
|
||||
|
||||
class ParallelFollowupResponse(BaseModel):
|
||||
"""Complete response schema for parallel follow-up PR review."""
|
||||
"""Complete response schema for parallel follow-up PR review.
|
||||
|
||||
Simplified schema — only fields that are consumed downstream are included.
|
||||
Removing unused fields reduces schema size and validation failure rate.
|
||||
"""
|
||||
|
||||
# Analysis metadata
|
||||
analysis_summary: str = Field(
|
||||
description="Brief summary of what was analyzed in this follow-up"
|
||||
)
|
||||
agents_invoked: list[str] = Field(
|
||||
default_factory=list,
|
||||
description="List of agent names that were invoked",
|
||||
)
|
||||
commits_analyzed: int = Field(0, description="Number of new commits analyzed")
|
||||
files_changed: int = Field(
|
||||
0, description="Number of files changed since last review"
|
||||
)
|
||||
|
||||
# Resolution verification (from resolution-verifier agent)
|
||||
resolution_verifications: list[ResolutionVerification] = Field(
|
||||
default_factory=list,
|
||||
description="AI-verified resolution status for each previous finding",
|
||||
description="Resolution status for each previous finding",
|
||||
)
|
||||
|
||||
# Finding validations (from finding-validator agent)
|
||||
finding_validations: list[FindingValidationResult] = Field(
|
||||
default_factory=list,
|
||||
description=(
|
||||
"Re-investigation results for unresolved findings. "
|
||||
"Validates whether findings are real issues or false positives."
|
||||
),
|
||||
description="Re-investigation results for unresolved findings",
|
||||
)
|
||||
|
||||
# New findings (from new-code-reviewer agent)
|
||||
new_findings: list[ParallelFollowupFinding] = Field(
|
||||
default_factory=list,
|
||||
description="New issues found in changes since last review",
|
||||
)
|
||||
|
||||
# Comment analysis (from comment-analyzer agent)
|
||||
comment_analyses: list[CommentAnalysis] = Field(
|
||||
default_factory=list,
|
||||
description="Analysis of contributor and AI comments",
|
||||
)
|
||||
comment_findings: list[ParallelFollowupFinding] = Field(
|
||||
default_factory=list,
|
||||
description="Issues identified from comment analysis",
|
||||
)
|
||||
|
||||
# Agent agreement tracking
|
||||
agent_agreement: AgentAgreement = Field(
|
||||
default_factory=AgentAgreement,
|
||||
description="Information about agent agreement on findings",
|
||||
)
|
||||
|
||||
# Verdict
|
||||
verdict: Literal[
|
||||
"READY_TO_MERGE", "MERGE_WITH_CHANGES", "NEEDS_REVISION", "BLOCKED"
|
||||
] = Field(description="Overall merge verdict")
|
||||
@@ -764,53 +683,17 @@ class ParallelFollowupResponse(BaseModel):
|
||||
|
||||
|
||||
class FindingValidationResult(BaseModel):
|
||||
"""
|
||||
Result of re-investigating an unresolved finding to validate it's actually real.
|
||||
|
||||
The finding-validator agent uses this to report whether a previous finding
|
||||
is a genuine issue or a false positive that should be dismissed.
|
||||
|
||||
EVIDENCE-BASED VALIDATION: No confidence scores - validation is binary.
|
||||
Either the evidence shows the issue exists, or it doesn't.
|
||||
"""
|
||||
"""Result of re-investigating an unresolved finding to determine if it's real."""
|
||||
|
||||
finding_id: str = Field(description="ID of the finding being validated")
|
||||
validation_status: Literal[
|
||||
"confirmed_valid", "dismissed_false_positive", "needs_human_review"
|
||||
] = Field(
|
||||
description=(
|
||||
"Validation result: "
|
||||
"confirmed_valid = code evidence proves issue IS real; "
|
||||
"dismissed_false_positive = code evidence proves issue does NOT exist; "
|
||||
"needs_human_review = cannot find definitive evidence either way"
|
||||
)
|
||||
)
|
||||
] = Field(description="Whether the finding is real, a false positive, or unclear")
|
||||
code_evidence: str = Field(
|
||||
min_length=1,
|
||||
description=(
|
||||
"REQUIRED: Exact code snippet examined from the file. "
|
||||
"Must be actual code copy-pasted from the file, not a description. "
|
||||
"This is the proof that determines the validation status."
|
||||
),
|
||||
)
|
||||
line_range: list[int] = Field(
|
||||
min_length=2,
|
||||
max_length=2,
|
||||
description="Start and end line numbers of the examined code [start, end]",
|
||||
description="Code snippet examined that supports the validation status",
|
||||
)
|
||||
explanation: str = Field(
|
||||
min_length=20,
|
||||
description=(
|
||||
"Detailed explanation connecting the code_evidence to the validation_status. "
|
||||
"Must explain: (1) what the original finding claimed, (2) what the actual code shows, "
|
||||
"(3) why this proves/disproves the issue."
|
||||
),
|
||||
)
|
||||
evidence_verified_in_file: bool = Field(
|
||||
description=(
|
||||
"True if the code_evidence was verified to exist at the specified line_range. "
|
||||
"False if the code couldn't be found (indicates hallucination in original finding)."
|
||||
)
|
||||
description="Why this finding was confirmed, dismissed, or flagged for human review",
|
||||
)
|
||||
|
||||
|
||||
|
||||
@@ -5,9 +5,8 @@ Tests for Finding Validation System
|
||||
Tests the finding-validator agent integration and FindingValidationResult models.
|
||||
This system prevents false positives from persisting by re-investigating unresolved findings.
|
||||
|
||||
NOTE: The validation system has been updated to use EVIDENCE-BASED validation
|
||||
instead of confidence scores. The key field is now `evidence_verified_in_file`
|
||||
which is a boolean indicating whether the code evidence was found at the specified location.
|
||||
NOTE: The validation system uses simplified models with finding_id, validation_status,
|
||||
code_evidence, and explanation fields.
|
||||
"""
|
||||
|
||||
import sys
|
||||
@@ -55,14 +54,11 @@ class TestFindingValidationResultModel:
|
||||
finding_id="SEC-001",
|
||||
validation_status="confirmed_valid",
|
||||
code_evidence="const query = `SELECT * FROM users WHERE id = ${userId}`;",
|
||||
line_range=(45, 45),
|
||||
explanation="SQL injection is present - user input is concatenated directly into the query.",
|
||||
evidence_verified_in_file=True,
|
||||
)
|
||||
assert result.finding_id == "SEC-001"
|
||||
assert result.validation_status == "confirmed_valid"
|
||||
assert "SELECT" in result.code_evidence
|
||||
assert result.evidence_verified_in_file is True
|
||||
|
||||
def test_valid_dismissed_false_positive(self):
|
||||
"""Test creating a dismissed_false_positive validation result."""
|
||||
@@ -70,12 +66,9 @@ class TestFindingValidationResultModel:
|
||||
finding_id="QUAL-002",
|
||||
validation_status="dismissed_false_positive",
|
||||
code_evidence="const sanitized = DOMPurify.sanitize(data);",
|
||||
line_range=(23, 26),
|
||||
explanation="Original finding claimed XSS but code uses DOMPurify.sanitize() for protection.",
|
||||
evidence_verified_in_file=True,
|
||||
)
|
||||
assert result.validation_status == "dismissed_false_positive"
|
||||
assert result.evidence_verified_in_file is True
|
||||
|
||||
def test_valid_needs_human_review(self):
|
||||
"""Test creating a needs_human_review validation result."""
|
||||
@@ -83,67 +76,39 @@ class TestFindingValidationResultModel:
|
||||
finding_id="LOGIC-003",
|
||||
validation_status="needs_human_review",
|
||||
code_evidence="async function handleRequest(req) { ... }",
|
||||
line_range=(100, 150),
|
||||
explanation="Race condition claim requires runtime analysis to verify.",
|
||||
evidence_verified_in_file=True,
|
||||
)
|
||||
assert result.validation_status == "needs_human_review"
|
||||
assert result.evidence_verified_in_file is True
|
||||
|
||||
def test_hallucinated_finding_not_verified(self):
|
||||
"""Test creating a result where evidence was not verified (hallucinated finding)."""
|
||||
def test_hallucinated_finding_dismissed(self):
|
||||
"""Test creating a result where finding was hallucinated."""
|
||||
result = FindingValidationResult(
|
||||
finding_id="HALLUC-001",
|
||||
validation_status="dismissed_false_positive",
|
||||
code_evidence="// Line 710 does not exist - file only has 600 lines",
|
||||
line_range=(600, 600),
|
||||
explanation="Original finding cited line 710 but file only has 600 lines. Hallucinated finding.",
|
||||
evidence_verified_in_file=False,
|
||||
)
|
||||
assert result.validation_status == "dismissed_false_positive"
|
||||
assert result.evidence_verified_in_file is False
|
||||
|
||||
def test_code_evidence_required(self):
|
||||
"""Test that code_evidence cannot be empty."""
|
||||
with pytest.raises(ValidationError) as exc_info:
|
||||
FindingValidationResult(
|
||||
finding_id="SEC-001",
|
||||
validation_status="confirmed_valid",
|
||||
code_evidence="", # Empty string should fail
|
||||
line_range=(45, 45),
|
||||
explanation="This is a detailed explanation of the issue.",
|
||||
evidence_verified_in_file=True,
|
||||
)
|
||||
errors = exc_info.value.errors()
|
||||
assert any("code_evidence" in str(e) for e in errors)
|
||||
def test_code_evidence_accepts_empty(self):
|
||||
"""Test that code_evidence accepts empty string (no min_length constraint)."""
|
||||
result = FindingValidationResult(
|
||||
finding_id="SEC-001",
|
||||
validation_status="confirmed_valid",
|
||||
code_evidence="",
|
||||
explanation="This is a detailed explanation of the issue.",
|
||||
)
|
||||
assert result.code_evidence == ""
|
||||
|
||||
def test_explanation_min_length(self):
|
||||
"""Test that explanation must be at least 20 characters."""
|
||||
with pytest.raises(ValidationError) as exc_info:
|
||||
FindingValidationResult(
|
||||
finding_id="SEC-001",
|
||||
validation_status="confirmed_valid",
|
||||
code_evidence="const x = 1;",
|
||||
line_range=(45, 45),
|
||||
explanation="Too short", # Less than 20 chars
|
||||
evidence_verified_in_file=True,
|
||||
)
|
||||
errors = exc_info.value.errors()
|
||||
assert any("explanation" in str(e) for e in errors)
|
||||
|
||||
def test_evidence_verified_required(self):
|
||||
"""Test that evidence_verified_in_file is required."""
|
||||
with pytest.raises(ValidationError) as exc_info:
|
||||
FindingValidationResult(
|
||||
finding_id="SEC-001",
|
||||
validation_status="confirmed_valid",
|
||||
code_evidence="const query = `SELECT * FROM users`;",
|
||||
line_range=(45, 45),
|
||||
explanation="SQL injection vulnerability found in the query construction.",
|
||||
# Missing evidence_verified_in_file
|
||||
)
|
||||
errors = exc_info.value.errors()
|
||||
assert any("evidence_verified_in_file" in str(e) for e in errors)
|
||||
def test_explanation_accepts_short_string(self):
|
||||
"""Test that explanation accepts short strings (no min_length constraint)."""
|
||||
result = FindingValidationResult(
|
||||
finding_id="SEC-001",
|
||||
validation_status="confirmed_valid",
|
||||
code_evidence="const x = 1;",
|
||||
explanation="Too short",
|
||||
)
|
||||
assert result.explanation == "Too short"
|
||||
|
||||
def test_invalid_validation_status(self):
|
||||
"""Test that invalid validation_status values are rejected."""
|
||||
@@ -152,9 +117,7 @@ class TestFindingValidationResultModel:
|
||||
finding_id="SEC-001",
|
||||
validation_status="invalid_status", # Not a valid status
|
||||
code_evidence="const x = 1;",
|
||||
line_range=(45, 45),
|
||||
explanation="This is a detailed explanation of the issue.",
|
||||
evidence_verified_in_file=True,
|
||||
)
|
||||
|
||||
|
||||
@@ -169,17 +132,13 @@ class TestFindingValidationResponse:
|
||||
finding_id="SEC-001",
|
||||
validation_status="confirmed_valid",
|
||||
code_evidence="const query = `SELECT * FROM users`;",
|
||||
line_range=(45, 45),
|
||||
explanation="SQL injection confirmed in this query.",
|
||||
evidence_verified_in_file=True,
|
||||
),
|
||||
FindingValidationResult(
|
||||
finding_id="QUAL-002",
|
||||
validation_status="dismissed_false_positive",
|
||||
code_evidence="const sanitized = DOMPurify.sanitize(data);",
|
||||
line_range=(23, 26),
|
||||
explanation="Code uses DOMPurify so XSS claim is false.",
|
||||
evidence_verified_in_file=True,
|
||||
),
|
||||
],
|
||||
summary="1 finding confirmed valid, 1 dismissed as false positive",
|
||||
@@ -194,10 +153,7 @@ class TestParallelFollowupResponseWithValidation:
|
||||
def test_response_includes_finding_validations(self):
|
||||
"""Test that ParallelFollowupResponse accepts finding_validations."""
|
||||
response = ParallelFollowupResponse(
|
||||
analysis_summary="Follow-up review with validation",
|
||||
agents_invoked=["resolution-verifier", "finding-validator"],
|
||||
commits_analyzed=3,
|
||||
files_changed=5,
|
||||
resolution_verifications=[
|
||||
ResolutionVerification(
|
||||
finding_id="SEC-001",
|
||||
@@ -210,13 +166,10 @@ class TestParallelFollowupResponseWithValidation:
|
||||
finding_id="SEC-001",
|
||||
validation_status="confirmed_valid",
|
||||
code_evidence="const query = `SELECT * FROM users`;",
|
||||
line_range=(45, 45),
|
||||
explanation="SQL injection confirmed in this query.",
|
||||
evidence_verified_in_file=True,
|
||||
)
|
||||
],
|
||||
new_findings=[],
|
||||
comment_analyses=[],
|
||||
comment_findings=[],
|
||||
verdict="NEEDS_REVISION",
|
||||
verdict_reasoning="1 confirmed valid security issue remains",
|
||||
@@ -227,10 +180,7 @@ class TestParallelFollowupResponseWithValidation:
|
||||
def test_response_with_dismissed_findings(self):
|
||||
"""Test response where findings are dismissed as false positives."""
|
||||
response = ParallelFollowupResponse(
|
||||
analysis_summary="All findings dismissed as false positives",
|
||||
agents_invoked=["resolution-verifier", "finding-validator"],
|
||||
commits_analyzed=3,
|
||||
files_changed=5,
|
||||
resolution_verifications=[
|
||||
ResolutionVerification(
|
||||
finding_id="SEC-001",
|
||||
@@ -243,13 +193,10 @@ class TestParallelFollowupResponseWithValidation:
|
||||
finding_id="SEC-001",
|
||||
validation_status="dismissed_false_positive",
|
||||
code_evidence="const query = db.prepare('SELECT * FROM users WHERE id = ?').get(userId);",
|
||||
line_range=(45, 48),
|
||||
explanation="Original review misread - using parameterized query.",
|
||||
evidence_verified_in_file=True,
|
||||
)
|
||||
],
|
||||
new_findings=[],
|
||||
comment_analyses=[],
|
||||
comment_findings=[],
|
||||
verdict="READY_TO_MERGE",
|
||||
verdict_reasoning="Previous finding was a false positive, now dismissed",
|
||||
@@ -350,31 +297,23 @@ class TestValidationIntegration:
|
||||
# so we verify the Pydantic models work correctly instead
|
||||
|
||||
response = ParallelFollowupResponse(
|
||||
analysis_summary="Follow-up with validation",
|
||||
agents_invoked=["resolution-verifier", "finding-validator"],
|
||||
commits_analyzed=3,
|
||||
files_changed=5,
|
||||
resolution_verifications=[],
|
||||
finding_validations=[
|
||||
FindingValidationResult(
|
||||
finding_id="SEC-001",
|
||||
validation_status="confirmed_valid",
|
||||
code_evidence="const query = `SELECT * FROM users`;",
|
||||
line_range=(45, 45),
|
||||
explanation="SQL injection confirmed in this query construction.",
|
||||
evidence_verified_in_file=True,
|
||||
),
|
||||
FindingValidationResult(
|
||||
finding_id="QUAL-002",
|
||||
validation_status="dismissed_false_positive",
|
||||
code_evidence="const sanitized = DOMPurify.sanitize(data);",
|
||||
line_range=(23, 26),
|
||||
explanation="Original XSS claim was incorrect - uses DOMPurify.",
|
||||
evidence_verified_in_file=True,
|
||||
),
|
||||
],
|
||||
new_findings=[],
|
||||
comment_analyses=[],
|
||||
comment_findings=[],
|
||||
verdict="READY_TO_MERGE",
|
||||
verdict_reasoning="1 dismissed as false positive, 1 confirmed valid but low severity",
|
||||
@@ -404,9 +343,7 @@ class TestValidationIntegration:
|
||||
finding_id="TEST-001",
|
||||
validation_status=status,
|
||||
code_evidence="const x = 1;",
|
||||
line_range=(1, 1),
|
||||
explanation="This is a valid explanation for the finding status.",
|
||||
evidence_verified_in_file=True,
|
||||
)
|
||||
assert result.validation_status == status
|
||||
|
||||
@@ -425,13 +362,10 @@ class TestEvidenceQuality:
|
||||
finding_id="SEC-001",
|
||||
validation_status="confirmed_valid",
|
||||
code_evidence="const query = db.query(`SELECT * FROM users WHERE id = ${userId}`);",
|
||||
line_range=(45, 45),
|
||||
explanation="SQL injection - user input interpolated directly into query string.",
|
||||
evidence_verified_in_file=True,
|
||||
)
|
||||
assert "SELECT" in result.code_evidence
|
||||
assert "userId" in result.code_evidence
|
||||
assert result.evidence_verified_in_file is True
|
||||
|
||||
def test_evidence_multiline_code_block(self):
|
||||
"""Test evidence spanning multiple lines."""
|
||||
@@ -443,11 +377,8 @@ class TestEvidenceQuality:
|
||||
finding_id="SEC-002",
|
||||
validation_status="confirmed_valid",
|
||||
code_evidence=multiline_evidence,
|
||||
line_range=(10, 14),
|
||||
explanation="SQL injection across multiple lines - user input flows into query.",
|
||||
evidence_verified_in_file=True,
|
||||
)
|
||||
assert result.line_range == [10, 14]
|
||||
assert "processInput" in result.code_evidence
|
||||
assert "userInput" in result.code_evidence
|
||||
|
||||
@@ -461,9 +392,7 @@ element.innerHTML = sanitized;"""
|
||||
finding_id="XSS-001",
|
||||
validation_status="dismissed_false_positive",
|
||||
code_evidence=context_evidence,
|
||||
line_range=(20, 24),
|
||||
explanation="XSS claim was false - code uses DOMPurify to sanitize before innerHTML.",
|
||||
evidence_verified_in_file=True,
|
||||
)
|
||||
assert "DOMPurify.sanitize" in result.code_evidence
|
||||
assert result.validation_status == "dismissed_false_positive"
|
||||
@@ -477,9 +406,7 @@ return transformedData;"""
|
||||
finding_id="LOGIC-001",
|
||||
validation_status="needs_human_review",
|
||||
code_evidence=ambiguous_evidence,
|
||||
line_range=(50, 53),
|
||||
explanation="Cannot determine if race condition exists - requires runtime analysis.",
|
||||
evidence_verified_in_file=True,
|
||||
)
|
||||
assert result.validation_status == "needs_human_review"
|
||||
|
||||
@@ -489,11 +416,8 @@ return transformedData;"""
|
||||
finding_id="HALLUC-001",
|
||||
validation_status="dismissed_false_positive",
|
||||
code_evidence="// File ends at line 200, original finding referenced line 500",
|
||||
line_range=(200, 200),
|
||||
explanation="Original finding cited non-existent line. File only has 200 lines.",
|
||||
evidence_verified_in_file=False,
|
||||
)
|
||||
assert result.evidence_verified_in_file is False
|
||||
assert result.validation_status == "dismissed_false_positive"
|
||||
|
||||
def test_evidence_with_special_characters(self):
|
||||
@@ -504,9 +428,7 @@ const encoded = str.replace(regex, (c) => `&#${c.charCodeAt(0)};`);"""
|
||||
finding_id="XSS-002",
|
||||
validation_status="dismissed_false_positive",
|
||||
code_evidence=special_evidence,
|
||||
line_range=(30, 31),
|
||||
explanation="XSS claim incorrect - code properly encodes special characters.",
|
||||
evidence_verified_in_file=True,
|
||||
)
|
||||
assert "<>" in result.code_evidence or "[<>" in result.code_evidence
|
||||
|
||||
@@ -516,9 +438,7 @@ const encoded = str.replace(regex, (c) => `&#${c.charCodeAt(0)};`);"""
|
||||
finding_id="SEC-003",
|
||||
validation_status="confirmed_valid",
|
||||
code_evidence="eval(userInput); // Execute user-provided code",
|
||||
line_range=(100, 100),
|
||||
explanation="Critical: eval() called on user input without sanitization. Remote code execution.",
|
||||
evidence_verified_in_file=True,
|
||||
)
|
||||
assert "eval" in result.code_evidence
|
||||
assert "userInput" in result.code_evidence
|
||||
@@ -530,9 +450,7 @@ const encoded = str.replace(regex, (c) => `&#${c.charCodeAt(0)};`);"""
|
||||
finding_id="LOGIC-002",
|
||||
validation_status="dismissed_false_positive",
|
||||
code_evidence="if (items.length === 0) { return []; } // Empty array handled",
|
||||
line_range=(75, 75),
|
||||
explanation="Original finding claimed missing empty array check, but line 75 shows check exists.",
|
||||
evidence_verified_in_file=True,
|
||||
)
|
||||
assert "length === 0" in result.code_evidence
|
||||
assert result.validation_status == "dismissed_false_positive"
|
||||
@@ -690,25 +608,19 @@ class TestScopeFiltering:
|
||||
finding_id="SEC-001",
|
||||
validation_status="confirmed_valid",
|
||||
code_evidence="eval(userInput);",
|
||||
line_range=(10, 10),
|
||||
explanation="Confirmed: eval on user input is a security risk.",
|
||||
evidence_verified_in_file=True,
|
||||
),
|
||||
FindingValidationResult(
|
||||
finding_id="QUAL-001",
|
||||
validation_status="dismissed_false_positive",
|
||||
code_evidence="const x = sanitize(input);",
|
||||
line_range=(20, 20),
|
||||
explanation="Dismissed: input is properly sanitized before use.",
|
||||
evidence_verified_in_file=True,
|
||||
),
|
||||
FindingValidationResult(
|
||||
finding_id="LOGIC-001",
|
||||
validation_status="needs_human_review",
|
||||
code_evidence="async function race() { ... }",
|
||||
line_range=(30, 35),
|
||||
explanation="Needs review: potential race condition requires runtime analysis.",
|
||||
evidence_verified_in_file=True,
|
||||
),
|
||||
]
|
||||
|
||||
@@ -720,41 +632,35 @@ class TestScopeFiltering:
|
||||
assert len(dismissed) == 1
|
||||
assert len(needs_review) == 1
|
||||
|
||||
def test_filter_validations_by_evidence_verified(self):
|
||||
"""Test filtering validation results by evidence verification status."""
|
||||
def test_filter_validations_by_status_type(self):
|
||||
"""Test filtering validation results by validation status type."""
|
||||
validations = [
|
||||
FindingValidationResult(
|
||||
finding_id="REAL-001",
|
||||
validation_status="confirmed_valid",
|
||||
code_evidence="const password = 'hardcoded';",
|
||||
line_range=(50, 50),
|
||||
explanation="Confirmed: hardcoded password found at specified location.",
|
||||
evidence_verified_in_file=True,
|
||||
),
|
||||
FindingValidationResult(
|
||||
finding_id="HALLUC-001",
|
||||
validation_status="dismissed_false_positive",
|
||||
code_evidence="// Line does not exist in file",
|
||||
line_range=(999, 999),
|
||||
explanation="Dismissed: original finding referenced non-existent line.",
|
||||
evidence_verified_in_file=False,
|
||||
),
|
||||
FindingValidationResult(
|
||||
finding_id="REAL-002",
|
||||
validation_status="dismissed_false_positive",
|
||||
code_evidence="const sanitized = escape(input);",
|
||||
line_range=(75, 75),
|
||||
explanation="Dismissed: code properly escapes input.",
|
||||
evidence_verified_in_file=True,
|
||||
),
|
||||
]
|
||||
|
||||
verified = [v for v in validations if v.evidence_verified_in_file]
|
||||
not_verified = [v for v in validations if not v.evidence_verified_in_file]
|
||||
confirmed = [v for v in validations if v.validation_status == "confirmed_valid"]
|
||||
dismissed = [v for v in validations if v.validation_status == "dismissed_false_positive"]
|
||||
|
||||
assert len(verified) == 2
|
||||
assert len(not_verified) == 1
|
||||
assert not_verified[0].finding_id == "HALLUC-001"
|
||||
assert len(confirmed) == 1
|
||||
assert len(dismissed) == 2
|
||||
assert confirmed[0].finding_id == "REAL-001"
|
||||
|
||||
def test_filter_findings_multiple_criteria(self):
|
||||
"""Test filtering findings with multiple criteria combined."""
|
||||
@@ -1191,25 +1097,19 @@ class TestFindingDeduplication:
|
||||
finding_id="SEC-001",
|
||||
validation_status="confirmed_valid",
|
||||
code_evidence="const query = `SELECT * FROM users`;",
|
||||
line_range=(45, 45),
|
||||
explanation="SQL injection confirmed - first validation.",
|
||||
evidence_verified_in_file=True,
|
||||
),
|
||||
FindingValidationResult(
|
||||
finding_id="SEC-001", # Same finding ID
|
||||
validation_status="confirmed_valid",
|
||||
code_evidence="const query = `SELECT * FROM users`;",
|
||||
line_range=(45, 45),
|
||||
explanation="SQL injection confirmed - duplicate validation.",
|
||||
evidence_verified_in_file=True,
|
||||
),
|
||||
FindingValidationResult(
|
||||
finding_id="SEC-002",
|
||||
validation_status="dismissed_false_positive",
|
||||
code_evidence="const sanitized = escape(input);",
|
||||
line_range=(60, 60),
|
||||
explanation="Input is properly escaped - false positive.",
|
||||
evidence_verified_in_file=True,
|
||||
),
|
||||
]
|
||||
|
||||
|
||||
@@ -393,9 +393,10 @@ class TestSecurityProfileIntegration:
|
||||
class TestGitCommitValidator:
|
||||
"""Tests for git commit validation (secret scanning)."""
|
||||
|
||||
def test_allows_normal_commit(self, temp_git_repo, stage_files):
|
||||
def test_allows_normal_commit(self, temp_git_repo, stage_files, monkeypatch):
|
||||
"""Allows commit without secrets."""
|
||||
stage_files({"normal.py": "x = 42\n"})
|
||||
monkeypatch.chdir(temp_git_repo)
|
||||
|
||||
allowed, reason = validate_git_commit("git commit -m 'test'")
|
||||
assert allowed is True
|
||||
|
||||
@@ -512,41 +512,47 @@ class TestVerificationEvidence:
|
||||
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."""
|
||||
def test_empty_code_examined_accepted(self):
|
||||
"""Test that empty code_examined is accepted (no min_length constraint)."""
|
||||
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)
|
||||
result = VerificationEvidence.model_validate(data)
|
||||
assert result.code_examined == ""
|
||||
|
||||
def test_invalid_line_range_rejected(self):
|
||||
"""Test that invalid line ranges are rejected."""
|
||||
def test_line_range_defaults_to_empty_list(self):
|
||||
"""Test that line_range_examined defaults to empty list when omitted."""
|
||||
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)
|
||||
result = VerificationEvidence.model_validate(data)
|
||||
assert result.line_range_examined == []
|
||||
|
||||
def test_invalid_verification_method_rejected(self):
|
||||
"""Test that invalid verification method is rejected."""
|
||||
def test_single_element_line_range_accepted(self):
|
||||
"""Test that single element line range is accepted (list[int])."""
|
||||
data = {
|
||||
"code_examined": "some code",
|
||||
"line_range_examined": [1],
|
||||
"verification_method": "direct_code_inspection",
|
||||
}
|
||||
result = VerificationEvidence.model_validate(data)
|
||||
assert result.line_range_examined == [1]
|
||||
|
||||
def test_custom_verification_method_accepted(self):
|
||||
"""Test that any string verification method is accepted."""
|
||||
data = {
|
||||
"code_examined": "some code",
|
||||
"line_range_examined": [1, 5],
|
||||
"verification_method": "guessed", # Invalid method
|
||||
"verification_method": "custom_method",
|
||||
}
|
||||
with pytest.raises(ValidationError) as exc_info:
|
||||
VerificationEvidence.model_validate(data)
|
||||
assert "verification_method" in str(exc_info.value)
|
||||
result = VerificationEvidence.model_validate(data)
|
||||
assert result.verification_method == "custom_method"
|
||||
|
||||
def test_all_verification_methods(self):
|
||||
"""Test all valid verification methods."""
|
||||
"""Test common verification methods."""
|
||||
methods = [
|
||||
"direct_code_inspection",
|
||||
"cross_file_trace",
|
||||
|
||||
Reference in New Issue
Block a user