feat(pr-review): evidence-based validation and trigger-driven exploration (#1593)
* 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * chore: remove duplicate .planning entries from .gitignore Co-Authored-By: Claude Opus 4.5 <[email protected]> * chore: remove docs/ from git tracking (already in .gitignore) Co-Authored-By: Claude Opus 4.5 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> --------- Co-authored-by: Claude Opus 4.5 <[email protected]>
This commit is contained in:
Andy
committed by
StillKnotKnown
StillKnotKnown
co-authored by
Claude Opus 4.5
Claude Opus 4.5
parent
e91dea5664
commit
a0ba50488b
+1
-1
@@ -173,4 +173,4 @@ OPUS_ANALYSIS_AND_IDEAS.md
|
||||
.security-key
|
||||
/shared_docs
|
||||
logs/security/
|
||||
Agents.md
|
||||
Agents.md
|
||||
@@ -466,6 +466,7 @@ def _get_token_from_macos_keychain(config_dir: str | None = None) -> str | None:
|
||||
if not (token.startswith("sk-ant-oat01-") or token.startswith("enc:")):
|
||||
return None
|
||||
|
||||
logger.debug(f"Found token in keychain service '{service_name}'")
|
||||
return token
|
||||
|
||||
except (subprocess.TimeoutExpired, json.JSONDecodeError, KeyError, Exception):
|
||||
@@ -500,6 +501,7 @@ def _get_token_from_windows_credential_files(
|
||||
token.startswith("sk-ant-oat01-")
|
||||
or token.startswith("enc:")
|
||||
):
|
||||
logger.debug(f"Found token in {cred_path}")
|
||||
return token
|
||||
# If config_dir provided but no token found, don't fall back to default
|
||||
return None
|
||||
@@ -595,6 +597,9 @@ def _get_token_from_linux_secret_service(config_dir: str | None = None) -> str |
|
||||
if token and (
|
||||
token.startswith("sk-ant-oat01-") or token.startswith("enc:")
|
||||
):
|
||||
logger.debug(
|
||||
f"Found token in secret service with label '{target_label}'"
|
||||
)
|
||||
return token
|
||||
except json.JSONDecodeError:
|
||||
continue
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
# Auto-Build Framework Dependencies
|
||||
# SDK 0.1.25+ required for improved tool use concurrency handling
|
||||
# Earlier versions had 400 errors when tool_use blocks had partial failures
|
||||
claude-agent-sdk>=0.1.25
|
||||
# SDK 0.1.22+ required for custom subagent support (bundles CLI 2.1.19+)
|
||||
# Earlier versions bundled CLI 2.1.9 which didn't properly register --agents flag
|
||||
claude-agent-sdk>=0.1.22
|
||||
python-dotenv>=1.0.0
|
||||
|
||||
# TOML parsing fallback for Python < 3.11
|
||||
|
||||
@@ -1050,8 +1050,102 @@ class PRContextGatherer:
|
||||
Returns:
|
||||
Empty set - LLM agents will discover dependents via Grep tool.
|
||||
"""
|
||||
# Return empty set - LLM agents will use Grep to find importers when needed
|
||||
return set()
|
||||
dependents: set[str] = set()
|
||||
path_obj = Path(file_path)
|
||||
stem = path_obj.stem # e.g., 'helpers' from 'utils/helpers.ts'
|
||||
|
||||
# NOTE: We no longer skip generic filenames like "utils", "types", "index".
|
||||
# The LLM-driven exploration system decides what's relevant based on the PR context.
|
||||
# Widely-used utilities are often the MOST important files to track dependents for.
|
||||
|
||||
# Build regex patterns and file extensions based on file type
|
||||
pattern = None
|
||||
file_extensions = []
|
||||
|
||||
if path_obj.suffix in [".ts", ".tsx", ".js", ".jsx"]:
|
||||
# Match various import styles for JS/TS
|
||||
# from './helpers', from '../utils/helpers', from '@/utils/helpers'
|
||||
# Escape stem for regex safety
|
||||
escaped_stem = re.escape(stem)
|
||||
pattern = re.compile(rf"['\"].*{escaped_stem}['\"]")
|
||||
file_extensions = [".ts", ".tsx", ".js", ".jsx"]
|
||||
elif path_obj.suffix == ".py":
|
||||
# Match Python imports: from .helpers import, import helpers
|
||||
escaped_stem = re.escape(stem)
|
||||
pattern = re.compile(rf"(from.*{escaped_stem}|import.*{escaped_stem})")
|
||||
file_extensions = [".py"]
|
||||
else:
|
||||
return dependents
|
||||
|
||||
# Directories to exclude
|
||||
exclude_dirs = {
|
||||
"node_modules",
|
||||
".git",
|
||||
"dist",
|
||||
"build",
|
||||
"__pycache__",
|
||||
".venv",
|
||||
"venv",
|
||||
}
|
||||
|
||||
# Walk the project directory
|
||||
project_path = Path(self.project_dir)
|
||||
files_checked = 0
|
||||
max_files_to_check = 2000 # Prevent infinite scanning on large codebases
|
||||
|
||||
try:
|
||||
for root, dirs, files in os.walk(project_path):
|
||||
# Modify dirs in-place to exclude certain directories
|
||||
dirs[:] = [d for d in dirs if d not in exclude_dirs]
|
||||
|
||||
for filename in files:
|
||||
# Check if we've hit the file limit
|
||||
if files_checked >= max_files_to_check:
|
||||
safe_print(
|
||||
f"[Context] File scan limit ({max_files_to_check}) reached for {file_path}. "
|
||||
f"Found {len(dependents)} dependents. "
|
||||
f"LLM agents can explore additional callers if needed via Read/Grep tools."
|
||||
)
|
||||
return dependents
|
||||
|
||||
# Check if file has the right extension
|
||||
if not any(filename.endswith(ext) for ext in file_extensions):
|
||||
continue
|
||||
|
||||
file_full_path = Path(root) / filename
|
||||
files_checked += 1
|
||||
|
||||
# Get relative path from project root
|
||||
try:
|
||||
relative_path = file_full_path.relative_to(project_path)
|
||||
relative_path_str = str(relative_path).replace("\\", "/")
|
||||
|
||||
# Don't include the file itself
|
||||
if relative_path_str == file_path:
|
||||
continue
|
||||
|
||||
# Search for the pattern in the file
|
||||
try:
|
||||
with open(
|
||||
file_full_path, encoding="utf-8", errors="ignore"
|
||||
) as f:
|
||||
content = f.read()
|
||||
if pattern.search(content):
|
||||
dependents.add(relative_path_str)
|
||||
if len(dependents) >= max_results:
|
||||
return dependents
|
||||
except (OSError, UnicodeDecodeError):
|
||||
# Skip files that can't be read
|
||||
continue
|
||||
|
||||
except ValueError:
|
||||
# File is not relative to project_path, skip it
|
||||
continue
|
||||
|
||||
except Exception as e:
|
||||
safe_print(f"[Context] Error finding dependents: {e}")
|
||||
|
||||
return dependents
|
||||
|
||||
def _prioritize_related_files(self, files: set[str], limit: int = 50) -> list[str]:
|
||||
"""
|
||||
|
||||
@@ -66,11 +66,7 @@ except (ImportError, ValueError, SystemError):
|
||||
PRReviewResult,
|
||||
ReviewSeverity,
|
||||
)
|
||||
from phase_config import (
|
||||
get_model_betas,
|
||||
get_thinking_kwargs_for_model,
|
||||
resolve_model_id,
|
||||
)
|
||||
from phase_config import get_thinking_budget, resolve_model_id
|
||||
from services.agent_utils import create_working_dir_injector
|
||||
from services.category_utils import map_category
|
||||
from services.io_utils import safe_print
|
||||
@@ -540,8 +536,7 @@ The SDK will run invoked agents in parallel automatically.
|
||||
spec_dir=self.github_dir,
|
||||
model=model,
|
||||
agent_type="pr_followup_parallel",
|
||||
betas=betas,
|
||||
fast_mode=self.config.fast_mode,
|
||||
max_thinking_tokens=thinking_budget,
|
||||
agents=self._define_specialist_agents(project_root),
|
||||
output_format={
|
||||
"type": "json_schema",
|
||||
|
||||
@@ -22,7 +22,6 @@ import hashlib
|
||||
import logging
|
||||
import os
|
||||
from collections import defaultdict
|
||||
from dataclasses import dataclass
|
||||
from pathlib import Path
|
||||
from typing import Any
|
||||
|
||||
@@ -58,7 +57,6 @@ try:
|
||||
AgentAgreement,
|
||||
FindingValidationResponse,
|
||||
ParallelOrchestratorResponse,
|
||||
SpecialistResponse,
|
||||
)
|
||||
from .sdk_utils import process_sdk_stream
|
||||
except (ImportError, ValueError, SystemError):
|
||||
@@ -74,12 +72,7 @@ except (ImportError, ValueError, SystemError):
|
||||
PRReviewResult,
|
||||
ReviewSeverity,
|
||||
)
|
||||
from phase_config import (
|
||||
get_model_betas,
|
||||
get_thinking_budget,
|
||||
get_thinking_kwargs_for_model,
|
||||
resolve_model_id,
|
||||
)
|
||||
from phase_config import get_thinking_budget, resolve_model_id
|
||||
from services.agent_utils import create_working_dir_injector
|
||||
from services.category_utils import map_category
|
||||
from services.io_utils import safe_print
|
||||
@@ -88,7 +81,6 @@ except (ImportError, ValueError, SystemError):
|
||||
AgentAgreement,
|
||||
FindingValidationResponse,
|
||||
ParallelOrchestratorResponse,
|
||||
SpecialistResponse,
|
||||
)
|
||||
from services.sdk_utils import process_sdk_stream
|
||||
|
||||
@@ -317,8 +309,9 @@ class ParallelOrchestratorReviewer:
|
||||
"Security specialist. Use for OWASP Top 10, authentication, "
|
||||
"injection, cryptographic issues, and sensitive data exposure. "
|
||||
"Invoke when PR touches auth, API endpoints, user input, database queries, "
|
||||
"or file operations. Use Read, Grep, and Glob tools to explore related files, "
|
||||
"callers, and tests as needed."
|
||||
"or file operations. IMPORTANT: Also check related files listed in the "
|
||||
"PR context - callers may be affected by security changes, and tests "
|
||||
"should verify security behavior."
|
||||
),
|
||||
prompt=with_working_dir(
|
||||
security_prompt, "You are a security expert. Find vulnerabilities."
|
||||
@@ -330,8 +323,9 @@ class ParallelOrchestratorReviewer:
|
||||
description=(
|
||||
"Code quality expert. Use for complexity, duplication, error handling, "
|
||||
"maintainability, and pattern adherence. Invoke when PR has complex logic, "
|
||||
"large functions, or significant business logic changes. Use Grep to search "
|
||||
"for similar patterns across the codebase for consistency checks."
|
||||
"large functions, or significant business logic changes. IMPORTANT: Check "
|
||||
"related files for pattern consistency - if a pattern is changed, similar "
|
||||
"code elsewhere should be updated too."
|
||||
),
|
||||
prompt=with_working_dir(
|
||||
quality_prompt,
|
||||
@@ -345,7 +339,8 @@ class ParallelOrchestratorReviewer:
|
||||
"Logic and correctness specialist. Use for algorithm verification, "
|
||||
"edge cases, state management, and race conditions. Invoke when PR has "
|
||||
"algorithmic changes, data transformations, concurrent operations, or bug fixes. "
|
||||
"Use Grep to find callers and dependents that may be affected by logic changes."
|
||||
"IMPORTANT: Check callers and dependents in related files - logic changes "
|
||||
"may break assumptions made by code that uses this file."
|
||||
),
|
||||
prompt=with_working_dir(
|
||||
logic_prompt, "You are a logic expert. Find correctness issues."
|
||||
@@ -358,7 +353,8 @@ class ParallelOrchestratorReviewer:
|
||||
"Codebase consistency expert. Use for naming conventions, ecosystem fit, "
|
||||
"architectural alignment, and avoiding reinvention. Invoke when PR introduces "
|
||||
"new patterns, large additions, or code that might duplicate existing functionality. "
|
||||
"Use Grep and Glob to explore existing patterns and conventions in the codebase."
|
||||
"IMPORTANT: Use related files to understand existing patterns - new code "
|
||||
"should match established conventions in the codebase."
|
||||
),
|
||||
prompt=with_working_dir(
|
||||
codebase_fit_prompt,
|
||||
@@ -387,7 +383,7 @@ class ParallelOrchestratorReviewer:
|
||||
"Reads the ACTUAL CODE at the finding location with fresh eyes. "
|
||||
"CRITICAL: Invoke for ALL findings after specialist agents complete. "
|
||||
"Can confirm findings as valid OR dismiss them as false positives. "
|
||||
"Use Read, Grep, and Glob to check for mitigations the original agent missed."
|
||||
"Check related files for mitigations the original agent missed."
|
||||
),
|
||||
prompt=with_working_dir(
|
||||
validator_prompt, "You validate whether findings are real issues."
|
||||
@@ -760,10 +756,80 @@ Found {len(context.ai_bot_comments)} comments from AI tools.
|
||||
{chr(10).join(commits_list)}
|
||||
"""
|
||||
|
||||
# Removed: Related files and import graph sections
|
||||
# LLM agents now discover relevant files themselves via Read, Grep, Glob tools
|
||||
# Build related files section (CONTEXT-02)
|
||||
related_files_section = ""
|
||||
if context.related_files:
|
||||
# Categorize by type
|
||||
tests = [
|
||||
f
|
||||
for f in context.related_files
|
||||
if ".test." in f
|
||||
or "_test." in f
|
||||
or f.startswith("test")
|
||||
or "/tests/" in f
|
||||
or "\\tests\\" in f
|
||||
]
|
||||
deps = [f for f in context.related_files if f not in tests]
|
||||
|
||||
# Limit to avoid context overflow
|
||||
tests = tests[:15]
|
||||
deps = deps[:15]
|
||||
|
||||
tests_str = ", ".join(f"`{t}`" for t in tests) if tests else "None found"
|
||||
deps_str = ", ".join(f"`{d}`" for d in deps) if deps else "None found"
|
||||
|
||||
related_files_section = f"""
|
||||
### Related Files to Investigate
|
||||
These files are related to the changes (imports, tests, dependents). **Pass relevant files to specialists when delegating.**
|
||||
|
||||
**Tests** ({len(tests)} files): {tests_str}
|
||||
**Dependencies/Callers** ({len(deps)} files): {deps_str}
|
||||
|
||||
**When delegating to specialists, include relevant files:**
|
||||
- **security-reviewer**: Mention files that handle the same data flow
|
||||
- **logic-reviewer**: Mention callers that depend on changed function signatures
|
||||
- **quality-reviewer**: Mention files with similar patterns for consistency check
|
||||
- **codebase-fit-reviewer**: Mention existing implementations of similar features
|
||||
|
||||
Example delegation: "Review the auth changes in login.ts. Also check auth_middleware.ts and auth.test.ts which use this module."
|
||||
"""
|
||||
|
||||
# Build import graph summary (CONTEXT-03)
|
||||
import_graph_section = ""
|
||||
import_entries = []
|
||||
changed_paths = {f.path for f in context.changed_files}
|
||||
|
||||
for file in context.changed_files[:10]: # Limit to 10 files
|
||||
# Find what this file imports (look for related files it references)
|
||||
imports_this = [
|
||||
r
|
||||
for r in context.related_files
|
||||
if r in (file.content or "") and r not in changed_paths
|
||||
][:5]
|
||||
# Find what imports this file (reverse deps in related_files)
|
||||
# Match by filename stem to catch imports without extension
|
||||
file_stem = file.path.split("/")[-1].split(".")[0]
|
||||
imported_by = [
|
||||
r
|
||||
for r in context.related_files
|
||||
if file_stem in r and r not in changed_paths
|
||||
][:5]
|
||||
|
||||
if imports_this or imported_by:
|
||||
entry = f"**{file.path}**"
|
||||
if imports_this:
|
||||
entry += f"\n - Imports: {', '.join(imports_this)}"
|
||||
if imported_by:
|
||||
entry += f"\n - Imported by: {', '.join(imported_by)}"
|
||||
import_entries.append(entry)
|
||||
|
||||
if import_entries:
|
||||
import_graph_section = f"""
|
||||
### Import Relationships
|
||||
How the changed files connect to the codebase:
|
||||
|
||||
{chr(10).join(import_entries[:20])}
|
||||
"""
|
||||
|
||||
pr_context = f"""
|
||||
---
|
||||
@@ -820,8 +886,7 @@ The SDK will run invoked agents in parallel automatically.
|
||||
spec_dir=self.github_dir,
|
||||
model=model,
|
||||
agent_type="pr_orchestrator_parallel",
|
||||
betas=betas,
|
||||
fast_mode=self.config.fast_mode,
|
||||
max_thinking_tokens=thinking_budget,
|
||||
agents=self._define_specialist_agents(project_root),
|
||||
output_format={
|
||||
"type": "json_schema",
|
||||
@@ -1090,9 +1155,30 @@ The SDK will run invoked agents in parallel automatically.
|
||||
else self.project_dir
|
||||
)
|
||||
|
||||
# Removed: Related files rescanning
|
||||
# LLM agents now discover relevant files themselves via Read, Grep, Glob tools
|
||||
# No need to pre-scan the codebase programmatically
|
||||
# Rescan for related files using the worktree/project root
|
||||
# This fixes the issue where related files were 0 because context gathering
|
||||
# happened BEFORE the worktree was created (PR files didn't exist locally)
|
||||
if context.changed_files:
|
||||
new_related_files = PRContextGatherer.find_related_files_for_root(
|
||||
context.changed_files,
|
||||
project_root,
|
||||
)
|
||||
# Always log rescan result (not gated by DEBUG_MODE)
|
||||
if new_related_files:
|
||||
context.related_files = new_related_files
|
||||
safe_print(
|
||||
f"[PRReview] Rescanned in worktree: found {len(new_related_files)} related files"
|
||||
)
|
||||
else:
|
||||
safe_print(
|
||||
f"[PRReview] Rescanned in worktree: found 0 related files "
|
||||
f"(initial scan found {len(context.related_files)})"
|
||||
)
|
||||
|
||||
# Build orchestrator prompt AFTER worktree creation and related files rescan
|
||||
prompt = self._build_orchestrator_prompt(context)
|
||||
# Capture agent definitions for debug logging (with worktree path)
|
||||
agent_defs = self._define_specialist_agents(project_root)
|
||||
|
||||
# Use model and thinking level from config (user settings)
|
||||
# Resolve model shorthand via environment variable override if configured
|
||||
@@ -1134,11 +1220,28 @@ The SDK will run invoked agents in parallel automatically.
|
||||
thinking_budget=thinking_budget,
|
||||
)
|
||||
|
||||
# Log results
|
||||
logger.info(
|
||||
f"[ParallelOrchestrator] Parallel specialists complete: "
|
||||
f"{len(findings)} findings from {len(agents_invoked)} agents"
|
||||
)
|
||||
# Process SDK stream with shared utility
|
||||
stream_result = await process_sdk_stream(
|
||||
client=client,
|
||||
context_name="ParallelOrchestrator",
|
||||
model=model,
|
||||
system_prompt=prompt,
|
||||
agent_definitions=agent_defs,
|
||||
)
|
||||
|
||||
# Check for stream processing errors
|
||||
if stream_result.get("error"):
|
||||
logger.error(
|
||||
f"[ParallelOrchestrator] SDK stream failed: {stream_result['error']}"
|
||||
)
|
||||
raise RuntimeError(
|
||||
f"SDK stream processing failed: {stream_result['error']}"
|
||||
)
|
||||
|
||||
result_text = stream_result["result_text"]
|
||||
structured_output = stream_result["structured_output"]
|
||||
agents_invoked = stream_result["agents_invoked"]
|
||||
msg_count = stream_result["msg_count"]
|
||||
|
||||
self._report_progress(
|
||||
"finalizing",
|
||||
@@ -1686,10 +1789,6 @@ The SDK will run invoked agents in parallel automatically.
|
||||
if not findings:
|
||||
return []
|
||||
|
||||
# Retry configuration for API errors
|
||||
MAX_VALIDATION_RETRIES = 2
|
||||
VALIDATOR_MAX_MESSAGES = 200 # Lower limit for validator (simpler task)
|
||||
|
||||
# Build validation prompt with all findings
|
||||
findings_json = []
|
||||
for f in findings:
|
||||
@@ -1729,100 +1828,55 @@ For EACH finding above:
|
||||
model_shorthand = self.config.model or "sonnet"
|
||||
model = resolve_model_id(model_shorthand)
|
||||
|
||||
# Retry loop for transient API errors
|
||||
last_error = None
|
||||
structured_output = None
|
||||
validation_succeeded = False
|
||||
for attempt in range(MAX_VALIDATION_RETRIES + 1):
|
||||
if attempt > 0:
|
||||
logger.info(
|
||||
f"[PRReview] Validation retry {attempt}/{MAX_VALIDATION_RETRIES}"
|
||||
)
|
||||
safe_print(
|
||||
f"[FindingValidator] Retry attempt {attempt}/{MAX_VALIDATION_RETRIES}"
|
||||
)
|
||||
# Create validator client (inherits worktree filesystem access)
|
||||
try:
|
||||
validator_client = create_client(
|
||||
project_dir=worktree_path,
|
||||
spec_dir=self.github_dir,
|
||||
model=model,
|
||||
agent_type="pr_finding_validator",
|
||||
max_thinking_tokens=get_thinking_budget("medium"),
|
||||
output_format={
|
||||
"type": "json_schema",
|
||||
"schema": FindingValidationResponse.model_json_schema(),
|
||||
},
|
||||
)
|
||||
except Exception as e:
|
||||
logger.error(f"[PRReview] Failed to create validator client: {e}")
|
||||
# Fail-safe: return original findings
|
||||
return findings
|
||||
|
||||
# Create validator client (inherits worktree filesystem access)
|
||||
try:
|
||||
# Get betas from model shorthand (before resolution to full ID)
|
||||
betas = get_model_betas(self.config.model or "sonnet")
|
||||
thinking_kwargs = get_thinking_kwargs_for_model(model, "medium")
|
||||
validator_client = create_client(
|
||||
project_dir=worktree_path,
|
||||
spec_dir=self.github_dir,
|
||||
# Run validation
|
||||
try:
|
||||
async with validator_client:
|
||||
await validator_client.query(prompt)
|
||||
|
||||
stream_result = await process_sdk_stream(
|
||||
client=validator_client,
|
||||
context_name="FindingValidator",
|
||||
model=model,
|
||||
agent_type="pr_finding_validator",
|
||||
betas=betas,
|
||||
fast_mode=self.config.fast_mode,
|
||||
output_format={
|
||||
"type": "json_schema",
|
||||
"schema": FindingValidationResponse.model_json_schema(),
|
||||
},
|
||||
**thinking_kwargs,
|
||||
system_prompt=prompt,
|
||||
)
|
||||
except Exception as e:
|
||||
logger.error(f"[PRReview] Failed to create validator client: {e}")
|
||||
last_error = e
|
||||
continue # Try again
|
||||
|
||||
# Run validation
|
||||
try:
|
||||
async with validator_client:
|
||||
await validator_client.query(prompt)
|
||||
|
||||
stream_result = await process_sdk_stream(
|
||||
client=validator_client,
|
||||
context_name="FindingValidator",
|
||||
model=model,
|
||||
system_prompt=prompt,
|
||||
max_messages=VALIDATOR_MAX_MESSAGES,
|
||||
if stream_result.get("error"):
|
||||
logger.error(
|
||||
f"[PRReview] Validation failed: {stream_result['error']}"
|
||||
)
|
||||
# Fail-safe: return original findings
|
||||
return findings
|
||||
|
||||
error = stream_result.get("error")
|
||||
if error:
|
||||
# Check for specific error types that warrant retry
|
||||
error_str = str(error).lower()
|
||||
is_retryable = (
|
||||
"400" in error_str
|
||||
or "concurrency" in error_str
|
||||
or "circuit breaker" in error_str
|
||||
or "tool_use" in error_str
|
||||
)
|
||||
structured_output = stream_result.get("structured_output")
|
||||
|
||||
if is_retryable and attempt < MAX_VALIDATION_RETRIES:
|
||||
logger.warning(
|
||||
f"[PRReview] Retryable validation error: {error}"
|
||||
)
|
||||
last_error = Exception(error)
|
||||
continue # Retry
|
||||
except Exception as e:
|
||||
logger.error(f"[PRReview] Validation stream error: {e}")
|
||||
# Fail-safe: return original findings
|
||||
return findings
|
||||
|
||||
logger.error(f"[PRReview] Validation failed: {error}")
|
||||
# Fail-safe: return original findings
|
||||
return findings
|
||||
|
||||
structured_output = stream_result.get("structured_output")
|
||||
|
||||
# Success - mark as succeeded and exit retry loop
|
||||
if structured_output:
|
||||
validation_succeeded = True
|
||||
break
|
||||
|
||||
except Exception as e:
|
||||
error_str = str(e).lower()
|
||||
is_retryable = (
|
||||
"400" in error_str
|
||||
or "concurrency" in error_str
|
||||
or "rate" in error_str
|
||||
)
|
||||
|
||||
if is_retryable and attempt < MAX_VALIDATION_RETRIES:
|
||||
logger.warning(f"[PRReview] Retryable stream error: {e}")
|
||||
last_error = e
|
||||
continue # Retry
|
||||
|
||||
logger.error(f"[PRReview] Validation stream error: {e}")
|
||||
# Fail-safe: return original findings
|
||||
return findings
|
||||
if not structured_output:
|
||||
logger.warning(
|
||||
"[PRReview] No structured validation output, keeping original findings"
|
||||
)
|
||||
return findings
|
||||
|
||||
# Parse validation results
|
||||
try:
|
||||
|
||||
@@ -537,51 +537,6 @@ class ValidationSummary(BaseModel):
|
||||
)
|
||||
|
||||
|
||||
class SpecialistFinding(BaseModel):
|
||||
"""A finding from a specialist agent (used in parallel SDK sessions)."""
|
||||
|
||||
severity: Literal["critical", "high", "medium", "low"] = Field(
|
||||
description="Issue severity level"
|
||||
)
|
||||
category: Literal[
|
||||
"security", "quality", "logic", "performance", "pattern", "test", "docs"
|
||||
] = Field(description="Issue category")
|
||||
title: str = Field(description="Brief issue title (max 80 chars)")
|
||||
description: str = Field(description="Detailed explanation of the issue")
|
||||
file: str = Field(description="File path where issue was found")
|
||||
line: int = Field(0, description="Line number of the issue")
|
||||
end_line: int | None = Field(None, description="End line number if multi-line")
|
||||
suggested_fix: str | None = Field(None, description="How to fix this issue")
|
||||
evidence: str = Field(
|
||||
min_length=1,
|
||||
description="Actual code snippet examined that shows the issue. Required.",
|
||||
)
|
||||
is_impact_finding: bool = Field(
|
||||
False,
|
||||
description="True if this is about affected code outside the PR (callers, dependencies)",
|
||||
)
|
||||
|
||||
|
||||
class SpecialistResponse(BaseModel):
|
||||
"""Response schema for individual specialist agent (parallel SDK sessions).
|
||||
|
||||
Used when each specialist runs as its own SDK session rather than via Task tool.
|
||||
"""
|
||||
|
||||
specialist_name: str = Field(
|
||||
description="Name of the specialist (security, quality, logic, codebase-fit)"
|
||||
)
|
||||
analysis_summary: str = Field(description="Brief summary of what was analyzed")
|
||||
files_examined: list[str] = Field(
|
||||
default_factory=list,
|
||||
description="List of files that were examined",
|
||||
)
|
||||
findings: list[SpecialistFinding] = Field(
|
||||
default_factory=list,
|
||||
description="Issues found during analysis",
|
||||
)
|
||||
|
||||
|
||||
class ParallelOrchestratorResponse(BaseModel):
|
||||
"""Complete response schema for parallel orchestrator PR review."""
|
||||
|
||||
|
||||
@@ -25,6 +25,243 @@ logger = logging.getLogger(__name__)
|
||||
# Check if debug mode is enabled
|
||||
DEBUG_MODE = os.environ.get("DEBUG", "").lower() in ("true", "1", "yes")
|
||||
|
||||
# ── TEMPORARY: Per-PR full agent communication logger (v2) ────────────
|
||||
# Writes every message to .auto-claude/github/pr/debug_logs/<context>_<ts>.log
|
||||
# Remove after measurement phase is complete.
|
||||
import datetime as _dt
|
||||
import json as _json
|
||||
from pathlib import Path as _Path
|
||||
|
||||
# Derive project root dynamically from this file's location
|
||||
# sdk_utils.py is at: apps/backend/runners/github/services/sdk_utils.py
|
||||
# So project root is 5 levels up
|
||||
_PROJECT_ROOT = _Path(__file__).resolve().parent.parent.parent.parent.parent
|
||||
_PR_LOG_DIR = _PROJECT_ROOT / ".auto-claude" / "github" / "pr" / "debug_logs"
|
||||
|
||||
|
||||
class _PRDebugLogger:
|
||||
"""Writes full agent communication to a log file for review.
|
||||
|
||||
Improvements (v2):
|
||||
- System prompt and agent definitions logged at session start
|
||||
- No truncation on thinking, text, tool input, or tool results
|
||||
- No duplicate logging (single structured dump per message)
|
||||
- Empty/whitespace content shown via repr()
|
||||
- Agent attribution via subagent_tool_ids mapping
|
||||
"""
|
||||
|
||||
def __init__(self, context_name: str, model: str | None = None):
|
||||
self._f = None
|
||||
self._subagent_tool_ids: dict[str, str] = {} # tool_id -> agent_name
|
||||
|
||||
try:
|
||||
_PR_LOG_DIR.mkdir(parents=True, exist_ok=True)
|
||||
ts = _dt.datetime.now().strftime("%Y%m%d_%H%M%S")
|
||||
self.path = _PR_LOG_DIR / f"{context_name}_{ts}.log"
|
||||
self._f = open(self.path, "w", encoding="utf-8")
|
||||
self._write(
|
||||
f"=== {context_name} Session Started at {_dt.datetime.now().isoformat()} ==="
|
||||
)
|
||||
if model:
|
||||
self._write(f"Model: {model}")
|
||||
self._write("")
|
||||
except OSError as e:
|
||||
# Failed to create directory or open file - logging disabled
|
||||
logger.warning(f"PR debug logger disabled: {e}")
|
||||
self.path = None
|
||||
|
||||
def _write(self, text: str):
|
||||
# Skip logging if file handle was not created successfully
|
||||
if self._f is None:
|
||||
return
|
||||
try:
|
||||
self._f.write(text + "\n")
|
||||
self._f.flush()
|
||||
except (OSError, ValueError) as e:
|
||||
# File write failed (file closed, disk full, etc.) - disable logging
|
||||
logger.warning(f"PR debug logger write failed: {e}")
|
||||
self._f = None
|
||||
|
||||
# ── Session preamble loggers ──────────────────────────────────────
|
||||
|
||||
def log_system_prompt(self, prompt: str):
|
||||
"""Log the full system prompt (no truncation)."""
|
||||
self._write(f"\n{'#' * 80}")
|
||||
self._write("# SYSTEM PROMPT (full orchestrator instructions + PR context)")
|
||||
self._write(f"# Length: {len(prompt)} chars")
|
||||
self._write(f"{'#' * 80}")
|
||||
self._write(prompt)
|
||||
self._write(f"{'#' * 80}\n")
|
||||
|
||||
def log_agent_definitions(self, agents: dict):
|
||||
"""Log all specialist agent definitions (prompts, tools, descriptions)."""
|
||||
self._write(f"\n{'#' * 80}")
|
||||
self._write(f"# AGENT DEFINITIONS ({len(agents)} specialists)")
|
||||
self._write(f"{'#' * 80}")
|
||||
for name, defn in agents.items():
|
||||
self._write(f"\n--- Agent: {name} ---")
|
||||
self._write(f" description: {getattr(defn, 'description', 'N/A')}")
|
||||
self._write(f" model: {getattr(defn, 'model', 'N/A')}")
|
||||
self._write(f" tools: {getattr(defn, 'tools', 'N/A')}")
|
||||
prompt = getattr(defn, "prompt", "")
|
||||
self._write(f" prompt ({len(prompt)} chars):")
|
||||
self._write(prompt)
|
||||
self._write(f"{'#' * 80}\n")
|
||||
|
||||
# ── Agent attribution ─────────────────────────────────────────────
|
||||
|
||||
def set_subagent_mapping(self, mapping: dict[str, str]):
|
||||
"""Update the tool_id -> agent_name mapping for attribution."""
|
||||
self._subagent_tool_ids = mapping
|
||||
|
||||
def _get_agent_label(self, tool_id: str) -> str:
|
||||
"""Return agent label if this tool_id belongs to a known subagent."""
|
||||
agent = self._subagent_tool_ids.get(tool_id)
|
||||
return f" [Agent:{agent}]" if agent else ""
|
||||
|
||||
# ── Per-message logger (single structured dump) ───────────────────
|
||||
|
||||
def log_message(self, msg_count: int, msg_type: str, msg: object):
|
||||
self._write(f"\n{'=' * 80}")
|
||||
self._write(f"--- Message #{msg_count} [{msg_type}] ---")
|
||||
self._write(f"{'=' * 80}")
|
||||
self._dump_raw(msg)
|
||||
|
||||
def _dump_raw(self, msg: object, indent: int = 0):
|
||||
"""Dump full raw message content recursively — NO truncation."""
|
||||
prefix = " " * indent
|
||||
# Content blocks
|
||||
if hasattr(msg, "content"):
|
||||
content = msg.content
|
||||
if isinstance(content, list):
|
||||
self._write(f"{prefix}[content] ({len(content)} blocks):")
|
||||
for i, block in enumerate(content):
|
||||
block_type = type(block).__name__
|
||||
self._write(f"{prefix} [{i}] {block_type}:")
|
||||
self._dump_block(block, indent + 2)
|
||||
elif isinstance(content, str):
|
||||
if not content or content.isspace():
|
||||
self._write(
|
||||
f"{prefix}[content] (string, {len(content)} chars): {repr(content)}"
|
||||
)
|
||||
else:
|
||||
self._write(f"{prefix}[content] (string, {len(content)} chars):")
|
||||
self._write(content)
|
||||
else:
|
||||
self._write(f"{prefix}[content] ({type(content).__name__}):")
|
||||
self._write(f"{prefix} {str(content)}")
|
||||
|
||||
# Role / type
|
||||
if hasattr(msg, "role"):
|
||||
self._write(f"{prefix}[role] {msg.role}")
|
||||
if hasattr(msg, "type") and not hasattr(msg, "content"):
|
||||
self._write(f"{prefix}[type] {msg.type}")
|
||||
|
||||
# Structured output
|
||||
if hasattr(msg, "structured_output") and msg.structured_output:
|
||||
self._write(f"{prefix}[structured_output]:")
|
||||
try:
|
||||
self._write(_json.dumps(msg.structured_output, indent=2, default=str))
|
||||
except Exception:
|
||||
self._write(f"{prefix} {str(msg.structured_output)}")
|
||||
|
||||
# Result message fields
|
||||
if hasattr(msg, "subtype"):
|
||||
self._write(f"{prefix}[subtype] {msg.subtype}")
|
||||
if hasattr(msg, "is_error"):
|
||||
self._write(f"{prefix}[is_error] {msg.is_error}")
|
||||
if hasattr(msg, "duration_ms"):
|
||||
self._write(f"{prefix}[duration_ms] {msg.duration_ms}")
|
||||
if hasattr(msg, "session_id"):
|
||||
self._write(f"{prefix}[session_id] {msg.session_id}")
|
||||
|
||||
# Catch-all for messages without content blocks
|
||||
for attr in ("text", "thinking", "name", "id", "input", "tool_use_id"):
|
||||
if hasattr(msg, attr) and not hasattr(msg, "content"):
|
||||
val = getattr(msg, attr)
|
||||
if val is not None:
|
||||
self._write(f"{prefix}[{attr}] {str(val)}")
|
||||
|
||||
def _dump_block(self, block: object, indent: int = 0):
|
||||
"""Dump a single content block — NO truncation."""
|
||||
prefix = " " * indent
|
||||
block_type = getattr(block, "type", type(block).__name__)
|
||||
|
||||
if block_type in ("text", "TextBlock") and hasattr(block, "text"):
|
||||
text = block.text
|
||||
if not text or text.isspace():
|
||||
self._write(f"{prefix}[text] ({len(text)} chars): {repr(text)}")
|
||||
else:
|
||||
self._write(f"{prefix}[text] ({len(text)} chars):")
|
||||
self._write(text)
|
||||
|
||||
elif block_type in ("thinking", "ThinkingBlock") and hasattr(block, "thinking"):
|
||||
text = block.thinking or getattr(block, "text", "")
|
||||
self._write(f"{prefix}[thinking] ({len(text)} chars):")
|
||||
self._write(text)
|
||||
|
||||
elif block_type in ("tool_use", "ToolUseBlock"):
|
||||
tool_name = getattr(block, "name", "unknown")
|
||||
tool_id = getattr(block, "id", "unknown")
|
||||
tool_input = getattr(block, "input", {})
|
||||
agent_label = self._get_agent_label(tool_id)
|
||||
self._write(f"{prefix}[tool_use] {tool_name} (id={tool_id}){agent_label}")
|
||||
try:
|
||||
self._write(_json.dumps(tool_input, indent=2, default=str))
|
||||
except Exception:
|
||||
self._write(str(tool_input))
|
||||
|
||||
elif block_type in ("tool_result", "ToolResultBlock"):
|
||||
tool_id = getattr(block, "tool_use_id", "unknown")
|
||||
is_error = getattr(block, "is_error", False)
|
||||
result = getattr(block, "content", "")
|
||||
if isinstance(result, list):
|
||||
result = " ".join(str(getattr(c, "text", c)) for c in result)
|
||||
status = "ERROR" if is_error else "OK"
|
||||
agent_label = self._get_agent_label(tool_id)
|
||||
self._write(
|
||||
f"{prefix}[tool_result] (tool_id={tool_id}) {status}{agent_label}"
|
||||
)
|
||||
self._write(str(result))
|
||||
|
||||
else:
|
||||
# Unknown block type — dump everything we can
|
||||
self._write(f"{prefix}[{block_type}] (raw dump):")
|
||||
for attr in dir(block):
|
||||
if not attr.startswith("_"):
|
||||
try:
|
||||
val = getattr(block, attr)
|
||||
if not callable(val):
|
||||
self._write(f"{prefix} {attr}: {str(val)}")
|
||||
except Exception:
|
||||
pass
|
||||
|
||||
# ── Structured output (standalone, for final result) ──────────────
|
||||
|
||||
def log_structured_output(self, output: dict):
|
||||
self._write("[STRUCTURED_OUTPUT]")
|
||||
try:
|
||||
self._write(_json.dumps(output, indent=2, default=str))
|
||||
except Exception:
|
||||
self._write(str(output))
|
||||
|
||||
# ── Session close ─────────────────────────────────────────────────
|
||||
|
||||
def close(self, summary: dict):
|
||||
self._write("\n=== Session Ended ===")
|
||||
self._write(f"Messages: {summary.get('msg_count', '?')}")
|
||||
self._write(f"Agents invoked: {summary.get('agents_invoked', [])}")
|
||||
self._write(f"Error: {summary.get('error')}")
|
||||
self._write(f"Log file: {self.path}")
|
||||
if self._f is not None:
|
||||
try:
|
||||
self._f.close()
|
||||
except OSError as e:
|
||||
logger.warning(f"PR debug logger close failed: {e}")
|
||||
|
||||
|
||||
# ── END TEMPORARY ──────────────────────────────────────────────────────
|
||||
|
||||
|
||||
def _short_model_name(model: str | None) -> str:
|
||||
"""Convert full model name to a short display name for logs.
|
||||
@@ -181,10 +418,8 @@ async def process_sdk_stream(
|
||||
on_structured_output: Callable[[dict[str, Any]], None] | None = None,
|
||||
context_name: str = "SDK",
|
||||
model: str | None = None,
|
||||
max_messages: int | None = None,
|
||||
# Deprecated parameters (kept for backwards compatibility, no longer used)
|
||||
system_prompt: str | None = None, # noqa: ARG001
|
||||
agent_definitions: dict | None = None, # noqa: ARG001
|
||||
system_prompt: str | None = None,
|
||||
agent_definitions: dict | None = None,
|
||||
) -> dict[str, Any]:
|
||||
"""
|
||||
Process SDK response stream with customizable callbacks.
|
||||
@@ -205,7 +440,8 @@ async def process_sdk_stream(
|
||||
on_structured_output: Callback for structured output - receives dict
|
||||
context_name: Name for logging (e.g., "ParallelOrchestrator", "ParallelFollowup")
|
||||
model: Model name for logging (e.g., "claude-sonnet-4-5-20250929")
|
||||
max_messages: Optional override for max message count circuit breaker (default: MAX_MESSAGE_COUNT)
|
||||
system_prompt: Full system prompt sent to the agent (logged at session start)
|
||||
agent_definitions: Dict of agent name -> AgentDefinition (logged at session start)
|
||||
|
||||
Returns:
|
||||
Dictionary with:
|
||||
@@ -230,6 +466,15 @@ async def process_sdk_stream(
|
||||
# Circuit breaker: max messages before aborting
|
||||
message_limit = max_messages if max_messages is not None else MAX_MESSAGE_COUNT
|
||||
|
||||
# TEMPORARY: per-PR debug file logger
|
||||
_dbg = _PRDebugLogger(context_name, model=model)
|
||||
|
||||
# Log session preamble: system prompt and agent definitions
|
||||
if system_prompt:
|
||||
_dbg.log_system_prompt(system_prompt)
|
||||
if agent_definitions:
|
||||
_dbg.log_agent_definitions(agent_definitions)
|
||||
|
||||
safe_print(f"[{context_name}] Processing SDK stream...")
|
||||
if DEBUG_MODE:
|
||||
safe_print(f"[DEBUG {context_name}] Awaiting response stream...")
|
||||
@@ -243,6 +488,7 @@ async def process_sdk_stream(
|
||||
try:
|
||||
msg_type = type(msg).__name__
|
||||
msg_count += 1
|
||||
_dbg.log_message(msg_count, msg_type, msg)
|
||||
|
||||
# CIRCUIT BREAKER: Abort if message count exceeds threshold
|
||||
# This prevents runaway retry loops (e.g., 400 errors causing infinite retries)
|
||||
@@ -322,6 +568,7 @@ async def process_sdk_stream(
|
||||
agents_invoked.append(agent_name)
|
||||
# Track this tool ID to log its result later
|
||||
subagent_tool_ids[tool_id] = agent_name
|
||||
_dbg.set_subagent_mapping(subagent_tool_ids)
|
||||
# Log with model info if available
|
||||
model_info = f" [{_short_model_name(model)}]" if model else ""
|
||||
safe_print(
|
||||
@@ -481,6 +728,7 @@ async def process_sdk_stream(
|
||||
# Only capture if we don't already have it (avoid duplicates)
|
||||
if structured_output is None:
|
||||
structured_output = msg.structured_output
|
||||
_dbg.log_structured_output(msg.structured_output)
|
||||
safe_print(f"[{context_name}] Received structured output")
|
||||
if on_structured_output:
|
||||
on_structured_output(msg.structured_output)
|
||||
@@ -553,14 +801,7 @@ async def process_sdk_stream(
|
||||
|
||||
safe_print(f"[{context_name}] Session ended. Total messages: {msg_count}")
|
||||
|
||||
# Set error flag if tool concurrency error was detected
|
||||
if detected_concurrency_error and not stream_error:
|
||||
stream_error = "tool_use_concurrency_error"
|
||||
logger.warning(
|
||||
f"[{context_name}] Tool use concurrency error detected - caller should retry"
|
||||
)
|
||||
|
||||
return {
|
||||
result = {
|
||||
"result_text": result_text,
|
||||
"structured_output": structured_output,
|
||||
"agents_invoked": agents_invoked,
|
||||
@@ -568,3 +809,6 @@ async def process_sdk_stream(
|
||||
"subagent_tool_ids": subagent_tool_ids,
|
||||
"error": stream_error,
|
||||
}
|
||||
_dbg.close(result)
|
||||
safe_print(f"[{context_name}] Full debug log: {_dbg.path}")
|
||||
return result
|
||||
|
||||
@@ -32,22 +32,7 @@ const RATE_LIMIT_INDICATORS = [
|
||||
* The patterns should only match actual API error messages.
|
||||
*/
|
||||
const AUTH_FAILURE_PATTERNS = [
|
||||
/authentication\s*(is\s*)?required/i,
|
||||
/not\s*(yet\s*)?authenticated/i,
|
||||
/login\s*(is\s*)?required/i,
|
||||
/oauth\s*token\s*(is\s*)?(invalid|expired|missing)/i,
|
||||
/unauthorized/i,
|
||||
/please\s*(log\s*in|login|authenticate)/i,
|
||||
/invalid\s*(credentials|token|api\s*key)/i,
|
||||
/auth(entication)?\s+(failed|error|failure)/i,
|
||||
/session\s*(expired|invalid)/i,
|
||||
/access\s*denied/i,
|
||||
/permission\s*denied/i,
|
||||
/401\s*unauthorized/i,
|
||||
/credentials\s*(are\s*)?(missing|invalid|expired)/i,
|
||||
// Match "OAuth token has expired" format from Claude API
|
||||
/oauth\s*token\s+has\s+expired/i,
|
||||
// Match Claude API authentication_error type in JSON responses
|
||||
// Match Claude API authentication_error type in JSON responses (most reliable)
|
||||
/["']?type["']?\s*:\s*["']?authentication_error["']?/i,
|
||||
// Match plain "API Error: 401" - this is a structured error format
|
||||
/API\s*Error:\s*401/i,
|
||||
|
||||
@@ -84,10 +84,11 @@ if _pydantic_was_mocked:
|
||||
|
||||
# Load agent_utils (shared utility for working directory injection)
|
||||
agent_utils_spec = importlib.util.spec_from_file_location(
|
||||
"agent_utils", backend_path / "runners" / "github" / "services" / "agent_utils.py"
|
||||
"agent_utils",
|
||||
backend_path / "runners" / "github" / "services" / "agent_utils.py"
|
||||
)
|
||||
agent_utils_module = importlib.util.module_from_spec(agent_utils_spec)
|
||||
sys.modules["services.agent_utils"] = agent_utils_module
|
||||
sys.modules['services.agent_utils'] = agent_utils_module
|
||||
agent_utils_spec.loader.exec_module(agent_utils_module)
|
||||
|
||||
# Load parallel_orchestrator_reviewer (contains _is_finding_in_scope and _cross_validate_findings)
|
||||
@@ -136,7 +137,6 @@ _is_finding_in_scope = orchestrator_module._is_finding_in_scope
|
||||
# Phase 5+ Tests: Scope Filtering (Updated)
|
||||
# =============================================================================
|
||||
|
||||
|
||||
class TestScopeFiltering:
|
||||
"""Test scope filtering logic (updated for Phase 5 - uses is_impact_finding schema field)."""
|
||||
|
||||
@@ -149,12 +149,11 @@ class TestScopeFiltering:
|
||||
ParallelOrchestratorFinding Pydantic model. The actual code uses
|
||||
getattr(finding, 'is_impact_finding', False) to access it.
|
||||
"""
|
||||
|
||||
def _make_finding(
|
||||
file: str = "src/test.py",
|
||||
line: int = 10,
|
||||
is_impact_finding: bool = False,
|
||||
**kwargs,
|
||||
**kwargs
|
||||
):
|
||||
defaults = {
|
||||
"id": "TEST001",
|
||||
@@ -170,7 +169,6 @@ class TestScopeFiltering:
|
||||
# Set is_impact_finding as attribute (accessed via getattr in _is_finding_in_scope)
|
||||
finding.is_impact_finding = is_impact_finding
|
||||
return finding
|
||||
|
||||
return _make_finding
|
||||
|
||||
def test_finding_in_changed_files_passes(self, make_finding):
|
||||
@@ -216,7 +214,7 @@ class TestScopeFiltering:
|
||||
file="src/utils.py",
|
||||
line=10,
|
||||
is_impact_finding=True, # Schema field replaces keyword detection
|
||||
description="This change breaks the helper function in utils.py",
|
||||
description="This change breaks the helper function in utils.py"
|
||||
)
|
||||
is_valid, _ = _is_finding_in_scope(finding, changed_files)
|
||||
assert is_valid
|
||||
@@ -230,7 +228,7 @@ class TestScopeFiltering:
|
||||
file="src/database.py",
|
||||
line=20,
|
||||
is_impact_finding=False,
|
||||
description="database.py depends on modified auth module",
|
||||
description="database.py depends on modified auth module"
|
||||
)
|
||||
is_valid, reason = _is_finding_in_scope(finding, changed_files)
|
||||
assert not is_valid
|
||||
@@ -453,12 +451,8 @@ class TestReverseDepDetection:
|
||||
# Method now intentionally returns empty set
|
||||
assert dependents == set()
|
||||
|
||||
def test_find_dependents_empty_for_any_file(self, tmp_path):
|
||||
"""Verify _find_dependents() returns empty for any input.
|
||||
|
||||
The LLM-driven architecture means agents decide what's relevant,
|
||||
not programmatic scanning.
|
||||
"""
|
||||
def test_generic_names_not_skipped(self, tmp_path):
|
||||
"""Generic names (index, main, utils) are no longer skipped - LLM decides relevance."""
|
||||
src_dir = tmp_path / "src"
|
||||
src_dir.mkdir()
|
||||
|
||||
@@ -468,11 +462,15 @@ class TestReverseDepDetection:
|
||||
gatherer = PRContextGathererIsolated(tmp_path, pr_number=1)
|
||||
dependents = gatherer._find_dependents("src/index.ts")
|
||||
|
||||
# Returns empty - LLM agents handle file discovery
|
||||
assert dependents == set()
|
||||
# Generic names should NOT be skipped anymore (behavior changed in Phase 4)
|
||||
# The LLM-driven system decides what's relevant based on PR context
|
||||
dependents_index = gatherer._find_dependents("src/index.ts")
|
||||
|
||||
def test_find_dependents_returns_set_type(self, tmp_path):
|
||||
"""Verify _find_dependents() returns correct type (set)."""
|
||||
# main.ts imports index, so it should be found as a dependent
|
||||
assert "src/main.ts" in dependents_index
|
||||
|
||||
def test_respects_file_limit(self, tmp_path):
|
||||
"""Large repo search should stop after reaching file limit."""
|
||||
src_dir = tmp_path / "src"
|
||||
src_dir.mkdir()
|
||||
(src_dir / "file.ts").write_text("export const x = 1;")
|
||||
|
||||
@@ -225,89 +225,6 @@ class TestLineNumberVerification:
|
||||
assert validator._is_line_relevant(line_content, finding)
|
||||
|
||||
|
||||
class TestFalsePositiveDetection:
|
||||
"""Test false positive detection."""
|
||||
|
||||
def test_vague_low_severity_filtered(self, validator):
|
||||
"""Test that vague low-severity findings are filtered."""
|
||||
finding = PRReviewFinding(
|
||||
id="STYLE001",
|
||||
severity=ReviewSeverity.LOW,
|
||||
category=ReviewCategory.STYLE,
|
||||
title="Code Could Be Improved",
|
||||
description="This code could be improved by considering using better practices.",
|
||||
file="src/utils.py",
|
||||
line=1,
|
||||
)
|
||||
|
||||
# This should fail _is_valid checks:
|
||||
# - Title too short (min 10 chars, this is 22 chars - OK)
|
||||
# - Description too short (min 30 chars, this is 73 chars - OK)
|
||||
# - Will be filtered due to low actionability score + short title description pattern
|
||||
result = validator.validate_findings([finding])
|
||||
assert len(result) == 0
|
||||
|
||||
def test_generic_without_fix_filtered(self, validator):
|
||||
"""Test that generic suggestions without fixes are filtered."""
|
||||
finding = PRReviewFinding(
|
||||
id="QUAL001",
|
||||
severity=ReviewSeverity.LOW,
|
||||
category=ReviewCategory.QUALITY,
|
||||
title="Improve This Code",
|
||||
description="This code should be improved for better quality and maintainability.",
|
||||
file="src/utils.py",
|
||||
line=1,
|
||||
suggested_fix="Fix it", # Too short (min 20 chars)
|
||||
)
|
||||
|
||||
# This should fail _is_valid checks:
|
||||
# - Suggested fix too short (min 20 chars)
|
||||
# - Will be filtered due to insufficient actionability
|
||||
result = validator.validate_findings([finding])
|
||||
assert len(result) == 0
|
||||
|
||||
def test_style_without_suggestion_filtered(self, validator):
|
||||
"""Test that style findings without good suggestions are filtered."""
|
||||
finding = PRReviewFinding(
|
||||
id="STYLE002",
|
||||
severity=ReviewSeverity.LOW,
|
||||
category=ReviewCategory.STYLE,
|
||||
title="Formatting Issue",
|
||||
description="The formatting of this code doesn't follow best practices and should be adjusted.",
|
||||
file="src/utils.py",
|
||||
line=1,
|
||||
suggested_fix="", # No suggestion
|
||||
)
|
||||
|
||||
# This should fail _is_valid checks:
|
||||
# - Suggested fix too short (empty, min 20 chars)
|
||||
# - Will be filtered due to insufficient actionability
|
||||
result = validator.validate_findings([finding])
|
||||
assert len(result) == 0
|
||||
|
||||
def test_specific_high_severity_not_filtered(self, validator):
|
||||
"""Test that specific high-severity findings are not filtered."""
|
||||
finding = PRReviewFinding(
|
||||
id="SEC001",
|
||||
severity=ReviewSeverity.HIGH,
|
||||
category=ReviewCategory.SECURITY,
|
||||
title="SQL Injection Vulnerability",
|
||||
description="The query construction uses f-strings which allows SQL injection. An attacker could inject malicious SQL code through the username parameter.",
|
||||
file="src/auth.py",
|
||||
line=13,
|
||||
suggested_fix="Use parameterized queries with placeholders instead of string formatting",
|
||||
)
|
||||
|
||||
# This should pass validation:
|
||||
# - Valid file and line
|
||||
# - Good title and description lengths
|
||||
# - Specific suggested fix
|
||||
# - High severity with actionability score
|
||||
result = validator.validate_findings([finding])
|
||||
assert len(result) == 1
|
||||
assert result[0].id == "SEC001"
|
||||
|
||||
|
||||
class TestActionabilityScoring:
|
||||
"""Test actionability scoring."""
|
||||
|
||||
@@ -398,18 +315,17 @@ class TestConfidenceThreshold:
|
||||
severity=ReviewSeverity.LOW,
|
||||
category=ReviewCategory.STYLE,
|
||||
title="Styl", # Very minimal (9 chars, just at min)
|
||||
description="Could be improved with better formatting here", # Vague pattern
|
||||
description="Could be improved with better formatting here",
|
||||
file="src/utils.py",
|
||||
line=1,
|
||||
suggested_fix="", # No fix
|
||||
)
|
||||
|
||||
# This should fail _is_valid checks:
|
||||
# - Title too short (min 10 chars, this is 9 chars)
|
||||
# - Suggested fix too short (empty, min 20 chars)
|
||||
# - Will be filtered before reaching threshold check
|
||||
result = validator.validate_findings([finding])
|
||||
assert len(result) == 0
|
||||
# Score check: low severity with no fix gets low actionability
|
||||
# With no fix, short title, and low severity: 0.5 (base) + 0.1 (file+line) = 0.6
|
||||
# This barely meets the 0.6 threshold for low severity
|
||||
score = validator._score_actionability(finding)
|
||||
assert score <= 0.6 # Low actionability due to missing suggested fix
|
||||
|
||||
|
||||
class TestFindingEnhancement:
|
||||
|
||||
@@ -37,6 +37,10 @@ from pydantic_models import (
|
||||
SecurityFinding,
|
||||
DeepAnalysisFinding,
|
||||
AICommentTriage,
|
||||
# Verification evidence models (Phase 2)
|
||||
VerificationEvidence,
|
||||
ParallelOrchestratorFinding,
|
||||
BaseFinding,
|
||||
)
|
||||
|
||||
|
||||
@@ -92,7 +96,7 @@ class TestFollowupFinding:
|
||||
"suggested_fix": "Use parameterized queries",
|
||||
"fixable": True,
|
||||
"verification": {
|
||||
"code_examined": "cursor.execute(f\"SELECT * FROM users WHERE id={user_input}\")",
|
||||
"code_examined": "query = 'SELECT * FROM users WHERE id=' + user_input",
|
||||
"line_range_examined": [42, 42],
|
||||
"verification_method": "direct_code_inspection",
|
||||
},
|
||||
@@ -114,8 +118,8 @@ class TestFollowupFinding:
|
||||
"description": "Function lacks documentation",
|
||||
"file": "utils.py",
|
||||
"verification": {
|
||||
"code_examined": "def my_function():",
|
||||
"line_range_examined": [10, 10],
|
||||
"code_examined": "def process_data(data):\n return data",
|
||||
"line_range_examined": [1, 2],
|
||||
"verification_method": "direct_code_inspection",
|
||||
},
|
||||
}
|
||||
@@ -172,8 +176,8 @@ class TestFollowupReviewResponse:
|
||||
"file": "service.py",
|
||||
"line": 100,
|
||||
"verification": {
|
||||
"code_examined": "def complex_method():",
|
||||
"line_range_examined": [100, 100],
|
||||
"code_examined": "def process(self, data):\n # 50 lines of nested if statements",
|
||||
"line_range_examined": [100, 150],
|
||||
"verification_method": "direct_code_inspection",
|
||||
},
|
||||
}
|
||||
@@ -245,9 +249,10 @@ class TestOrchestratorFinding:
|
||||
"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, 25],
|
||||
"line_range_examined": [25, 26],
|
||||
"verification_method": "direct_code_inspection",
|
||||
},
|
||||
}
|
||||
@@ -265,8 +270,8 @@ class TestOrchestratorFinding:
|
||||
"category": "quality",
|
||||
"severity": "low",
|
||||
"verification": {
|
||||
"code_examined": "pass",
|
||||
"line_range_examined": [1, 1],
|
||||
"code_examined": "def test():\n pass",
|
||||
"line_range_examined": [1, 2],
|
||||
"verification_method": "direct_code_inspection",
|
||||
},
|
||||
}
|
||||
@@ -290,6 +295,7 @@ class TestOrchestratorReviewResponse:
|
||||
"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],
|
||||
@@ -397,7 +403,7 @@ class TestSecurityFinding:
|
||||
"file": "template.html",
|
||||
"line": 50,
|
||||
"verification": {
|
||||
"code_examined": "innerHTML = user_input",
|
||||
"code_examined": "<div>{{ user_input }}</div>",
|
||||
"line_range_examined": [50, 50],
|
||||
"verification_method": "direct_code_inspection",
|
||||
},
|
||||
@@ -419,6 +425,7 @@ class TestDeepAnalysisFinding:
|
||||
"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],
|
||||
@@ -437,10 +444,11 @@ class TestDeepAnalysisFinding:
|
||||
"description": "Could not verify behavior",
|
||||
"file": "lib.py",
|
||||
"category": "verification_failed",
|
||||
"verification_note": "Unable to find test coverage",
|
||||
"verification": {
|
||||
"code_examined": "pass",
|
||||
"line_range_examined": [1, 1],
|
||||
"verification_method": "direct_code_inspection",
|
||||
"code_examined": "def some_function():\n return process_data()",
|
||||
"line_range_examined": [1, 2],
|
||||
"verification_method": "cross_file_trace",
|
||||
},
|
||||
}
|
||||
result = DeepAnalysisFinding.model_validate(data)
|
||||
@@ -480,3 +488,220 @@ class TestAICommentTriage:
|
||||
}
|
||||
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"]
|
||||
|
||||
Reference in New Issue
Block a user