From f823a82d6b6470820e4a75a172c94b745e230846 Mon Sep 17 00:00:00 2001 From: Siddharth Date: Sun, 24 May 2026 10:55:14 +0530 Subject: [PATCH 1/4] fix(scoring): replace coverage penalty with findings-based verify scoring Coverage penalty (apply_coverage_penalty) was adding up to 50 phantom points based on unreviewed file count rather than actual issues found. This inflated scores on both first-time reviews and verify-only runs. - Remove apply_coverage_penalty from PRScorer entirely - Add calculate_verify_score for verify-only runs: penalty = sum of still_present findings by severity/category, fixed findings = 0 - Enrich fix_verifications with severity/category from prior findings in review_job._write_findings so post_findings can score them - Coverage stats still tracked and displayed, just not penalized --- src/batch_review_job.py | 40 ++++++++++++++ src/models/review_models.py | 2 + src/post_findings.py | 14 +++-- src/pr_scorer.py | 64 +++++++++++++++++----- src/review_job.py | 88 +++++++++++++++++++++++++++++- tests/unit/test_coverage_system.py | 25 +-------- 6 files changed, 189 insertions(+), 44 deletions(-) diff --git a/src/batch_review_job.py b/src/batch_review_job.py index 38ae40b..3331ae4 100644 --- a/src/batch_review_job.py +++ b/src/batch_review_job.py @@ -88,6 +88,12 @@ def run(self, dry_run: bool = False, commit_id: str = "") -> Dict[str, Any]: if previous_findings: logger.info("Re-push detected: %d previous findings", len(previous_findings)) + # --- Step 2d: Re-push → verify fixes only (cheap), skip full review --- + if previous_findings: + return self._run_verify_only( + previous_findings, dry_run=dry_run, commit_id=commit_id, + ) + # --- Step 3: Build graph once --- graph_store = self._build_graph(len(code_files)) @@ -191,6 +197,40 @@ def _fetch_pr_details(self): logger.warning("PR pre-fetch failed: %s", exc) return None + _VERIFY_MODEL = "gpt-5-codex" + _VERIFY_MAX_TURNS = 5 + + def _run_verify_only( + self, + previous_findings: list, + dry_run: bool = False, + commit_id: str = "", + ) -> Dict[str, Any]: + """Run fix verification only (no full review). Cheap agent-based path for re-pushes. + + Uses a ReviewJob in VERIFY_FIXES mode with gpt-4o-mini and a low turn + budget. The agent has tools (read_local_file, search_code) so it can + investigate cross-file fixes — unlike the deterministic fix_verifier. + """ + logger.info( + "Running fix verification agent (%s, max_turns=%d) for %d prior findings", + self._VERIFY_MODEL, self._VERIFY_MAX_TURNS, len(previous_findings), + ) + + config = ReviewJobConfig( + pr_id=self.pr_id, + repo=self.repo, + workspace=self.workspace, + model=self._VERIFY_MODEL, + max_turns=self._VERIFY_MAX_TURNS, + prompt_path=self.prompt_path, + vcs=self.vcs, + previous_findings=previous_findings, + review_mode=ReviewMode.VERIFY_FIXES, + ) + job = ReviewJob(config, settings=self.settings) + return job.run(dry_run=dry_run, commit_id=commit_id) + def _fetch_previous_findings(self) -> list: """Fetch existing review threads with cr_id markers.""" try: diff --git a/src/models/review_models.py b/src/models/review_models.py index 054a3b0..6625008 100644 --- a/src/models/review_models.py +++ b/src/models/review_models.py @@ -221,6 +221,8 @@ class FixVerification: cr_id: str # matches a Finding.id from prior review status: str # "fixed" | "still_present" | "not_relevant" reason: str # human-readable explanation + severity: Optional[str] = None # from original finding: "critical" | "warning" | "suggestion" + category: Optional[str] = None # from original finding: "security" | "performance" | etc. @dataclass diff --git a/src/post_findings.py b/src/post_findings.py index 283f7c6..43d629d 100644 --- a/src/post_findings.py +++ b/src/post_findings.py @@ -288,6 +288,8 @@ def _parse_findings_file(data: dict): cr_id=fv.get("cr_id", "unknown"), status=fv.get("status", "not_relevant"), reason=fv.get("reason", ""), + severity=fv.get("severity"), + category=fv.get("category"), )) except Exception as exc: logger.warning("Skipping unparseable fix_verification: %s", exc) @@ -1195,9 +1197,12 @@ def run( new_findings = [f for f in capped if f.id not in posted_cr_ids] deduped_count = len(capped) - len(new_findings) - # 8. Score (use mode-adjusted findings) - all_adjusted = scorer.apply_mode_multipliers(capped, findings_file.review_modes) - score = scorer.calculate_pr_score(all_adjusted) + # 8. Score + if is_verify_only and findings_file.fix_verifications: + score = scorer.calculate_verify_score(findings_file.fix_verifications) + else: + all_adjusted = scorer.apply_mode_multipliers(capped, findings_file.review_modes) + score = scorer.calculate_pr_score(all_adjusted) # 9. Post inline comments (skip entirely for verify-only) posted_count = 0 @@ -1260,8 +1265,7 @@ def run( coverage_ratio = 1.0 coverage_gate_mode = getattr(settings, "coverage_gate_mode", "hard") if settings else "hard" - # 11c. Apply coverage penalty to score (only when coverage tracking is active) - score = scorer.apply_coverage_penalty(score, coverage_ratio) + # 11c. Coverage is tracked for display but does not affect the penalty score # 12. Gate evaluation from .codereview.yml (verify-only always passes — no new findings) gate_config = _load_codereview_yml(workspace) diff --git a/src/pr_scorer.py b/src/pr_scorer.py index e93c001..99b7a11 100644 --- a/src/pr_scorer.py +++ b/src/pr_scorer.py @@ -259,25 +259,59 @@ def _extract_severity_counts(self, statistics: Dict[str, int]) -> Dict[str, int] 'good': statistics.get('good', 0) } - def apply_coverage_penalty(self, score: PRScore, coverage_ratio: float) -> PRScore: + def calculate_verify_score(self, fix_verifications) -> PRScore: + """Calculate score for verify-only runs from prior findings' severity/category. + + Penalty = sum of penalties for all prior findings that are still_present. + Fixed findings contribute zero penalty. """ - Add a coverage penalty to an existing PRScore. + if not self.enable_scoring: + return self._create_disabled_score() - For coverage below 100%, adds: (1 - coverage_ratio) * 50 penalty points. - This ensures incomplete reviews score poorly even when the gate mode is 'log'. + total_penalty = 0.0 + original_penalty = 0.0 + category_penalties: Dict[str, float] = {} + stats: Dict[str, int] = {'critical': 0, 'warning': 0, 'suggestion': 0, 'good': 0} - Args: - score: Existing PRScore to augment. - coverage_ratio: Fraction of code files reviewed (0.0–1.0). + for fv in fix_verifications: + sev = fv.severity or "warning" + cat = fv.category or "best_practices" + issue_penalty = self._calculate_issue_penalty(sev, cat) + original_penalty += issue_penalty - Returns: - New PRScore with coverage penalty applied. - """ - if coverage_ratio >= 1.0: - return score - penalty = round((1.0 - coverage_ratio) * 50.0, 1) - from dataclasses import replace - return replace(score, total_penalty=round(score.total_penalty + penalty, 1)) + if fv.status == "still_present": + total_penalty += issue_penalty + category_penalties[cat] = category_penalties.get(cat, 0.0) + issue_penalty + stats[sev] = stats.get(sev, 0) + 1 + + category_penalties = {k: round(v, 1) for k, v in category_penalties.items() if v > 0} + total_penalty = round(total_penalty, 1) + + overall_stars = self._penalty_to_stars(total_penalty) + quality_level = self._get_quality_level(total_penalty) + category_stars = { + cat: self._penalty_to_stars(p) for cat, p in category_penalties.items() + } + + fixed_count = sum(1 for fv in fix_verifications if fv.status == "fixed") + total_count = len(fix_verifications) + breakdown = [ + f"Original penalty: {original_penalty:.1f} points ({total_count} prior findings)", + f"Fixed: {fixed_count} findings (penalty removed)", + f"Still present: {total_count - fixed_count} findings", + f"Current penalty: {total_penalty:.1f} points", + f"Quality level: {quality_level}", + ] + + return PRScore( + total_penalty=total_penalty, + overall_stars=overall_stars, + category_penalties=category_penalties, + category_stars=category_stars, + issues_by_severity=self._extract_severity_counts(stats), + scoring_breakdown=breakdown, + quality_level=quality_level, + ) def _create_disabled_score(self) -> PRScore: return PRScore( diff --git a/src/review_job.py b/src/review_job.py index 60e8e41..82de75a 100644 --- a/src/review_job.py +++ b/src/review_job.py @@ -191,6 +191,11 @@ def create_findings(self) -> Path: except Exception as exc: logger.debug("Previous findings fetch skipped: %s", exc) + # VERIFY_FIXES: lightweight path — no diffs, no graph, just the + # previous findings table + tools so the agent can investigate. + if self.config.review_mode == ReviewMode.VERIFY_FIXES: + return self._create_verify_findings() + changed_files = [] pr_details = None skipped_count = 0 @@ -254,6 +259,13 @@ def create_findings(self) -> Path: two_pass_enabled = getattr(self.settings, "two_pass_enabled", True) + # VERIFY_FIXES mode must use single-pass — the two-pass scan/verify + # pipeline looks for NEW candidates, but verify-only needs the full + # prompt with previous findings + fix_verifications instructions. + if self.config.review_mode == ReviewMode.VERIFY_FIXES: + two_pass_enabled = False + logger.info("VERIFY_FIXES mode — forcing single-pass") + if two_pass_enabled: try: self._agent_result = self._run_two_pass( @@ -288,6 +300,51 @@ def create_findings(self) -> Path: return self._findings_path + def _create_verify_findings(self) -> Path: + """Lightweight VERIFY_FIXES path — no diffs, no graph, just tools + prior findings.""" + prompt = self._build_fix_verify_prompt() + logger.info("VERIFY_FIXES prompt: %d chars", len(prompt)) + + runner = OpenAIAgentRunner( + settings=self.settings, + workspace=self.config.workspace, + model=self.config.model, + pr_id=self.config.pr_id, + repo=self.config.repo, + ) + self._agent_result = runner.run(prompt, max_turns=self.config.max_turns) + + if not self._agent_result.findings_data: + import warnings + warnings.warn( + "Verify agent did not produce findings JSON; empty fix_verifications.", + stacklevel=2, + ) + + self._stamp_usage(self._agent_result) + self._write_findings(self._agent_result.findings_data) + return self._findings_path + + def _build_fix_verify_prompt(self) -> str: + """Build a minimal prompt for VERIFY_FIXES — just the base instructions, + previous findings table, and verify-only constraints. No diffs injected.""" + if self.config.prompt_text: + text = self.config.prompt_text + else: + text = self.config.prompt_path.read_text(encoding="utf-8") + + ws_posix = str(self.config.workspace).replace("\\", "/") + text = text.replace("/workspace/", ws_posix + "/") + text = text.replace("$PR_ID", str(self.config.pr_id)) + text = text.replace("$REPO", self.config.repo) + text = text.replace("$VCS", self.config.vcs) + + if self.config.previous_findings: + text += self._build_previous_findings_section(self.config.previous_findings) + + text += self._build_verify_only_instructions() + return text + # ------------------------------------------------------------------ # Phase 2 — score, gate, post comments # ------------------------------------------------------------------ @@ -523,7 +580,21 @@ def _build_verify_only_instructions(self) -> str: "- `findings[]` MUST be empty — do NOT add new findings\n" "- Populate `fix_verifications[]` for EVERY cr_id in the table above\n" "- Skip Steps 3-5 of the standard review process\n" - "- Each `fix_verifications` entry requires: `cr_id`, `status` (fixed/still_present/not_relevant), `reason`\n" + "- Each `fix_verifications` entry requires: `cr_id`, `status`, `reason`\n\n" + "### Status definitions (use EXACTLY these values)\n" + "- `fixed` — the issue described in the finding has been resolved " + "(code was changed, test was added, bug was fixed — the problem no longer exists)\n" + "- `still_present` — the issue still exists in the current code\n" + "- `not_relevant` — the file was deleted or the finding no longer applies " + "because the relevant code was entirely removed (NOT for issues that were fixed)\n\n" + "**IMPORTANT:** If a finding was addressed/fixed by the developer, use `fixed` — " + "do NOT use `not_relevant`. Reserve `not_relevant` only for deleted files or " + "removed code paths.\n\n" + "### How to verify\n" + "- Use `read_local_file` to read the current file contents from the workspace\n" + "- Use `search_code` to search for patterns across the codebase (e.g. test files)\n" + "- Do NOT use `get_file_content` or `get_file_diff` — the workspace is already cloned locally\n" + "- Do NOT call `get_pr` — all prior findings are in the table above\n" ) def _build_check_new_instructions(self) -> str: @@ -1328,6 +1399,20 @@ def _stamp_usage(self, result: AgentResult): result.findings_data.setdefault("tool_calls", result.tool_calls_count) result.findings_data.setdefault("agent", "openai-api") + def _enrich_fix_verifications(self, data: dict): + """Stamp severity/category from previous_findings onto each fix_verification entry.""" + prev = self.config.previous_findings or [] + if not prev: + return + by_cr_id = {f.cr_id: f for f in prev if f.cr_id} + for fv in data.get("fix_verifications", []): + if not isinstance(fv, dict): + continue + prior = by_cr_id.get(fv.get("cr_id")) + if prior: + fv.setdefault("severity", prior.severity) + fv.setdefault("category", prior.category) + def _write_findings(self, data: dict): data["pr_id"] = self.config.pr_id data["repo"] = self.config.repo @@ -1339,6 +1424,7 @@ def _write_findings(self, data: dict): data["findings"] = [] if "verify_fixes" not in review_modes: review_modes.append("verify_fixes") + self._enrich_fix_verifications(data) elif self.config.review_mode == ReviewMode.CHECK_NEW: data["fix_verifications"] = [] if "check_new" not in review_modes: diff --git a/tests/unit/test_coverage_system.py b/tests/unit/test_coverage_system.py index 6b81e37..3953c89 100644 --- a/tests/unit/test_coverage_system.py +++ b/tests/unit/test_coverage_system.py @@ -277,31 +277,10 @@ def _make_scorer(self): } return PRScorer(penalty_matrix=matrix, star_thresholds=[0.0, 5.0, 15.0, 30.0, 50.0]) - def test_full_coverage_no_penalty(self): - scorer = self._make_scorer() - from models.review_models import Finding - score = scorer.calculate_pr_score([]) - penalized = scorer.apply_coverage_penalty(score, coverage_ratio=1.0) - assert penalized.total_penalty == score.total_penalty - - def test_zero_coverage_adds_50_penalty(self): - scorer = self._make_scorer() - score = scorer.calculate_pr_score([]) - penalized = scorer.apply_coverage_penalty(score, coverage_ratio=0.0) - assert penalized.total_penalty == 50.0 - - def test_half_coverage_adds_25_penalty(self): - scorer = self._make_scorer() - score = scorer.calculate_pr_score([]) - penalized = scorer.apply_coverage_penalty(score, coverage_ratio=0.5) - assert penalized.total_penalty == 25.0 - - def test_625_coverage_correct_penalty(self): + def test_score_based_on_findings_only(self): scorer = self._make_scorer() score = scorer.calculate_pr_score([]) - penalized = scorer.apply_coverage_penalty(score, coverage_ratio=0.625) - expected = round((1.0 - 0.625) * 50, 1) - assert penalized.total_penalty == expected + assert score.total_penalty == 0.0 # --------------------------------------------------------------------------- From 903f93f1acc8fd0a629fc8519dd349c7e1e9d6ff Mon Sep 17 00:00:00 2001 From: Siddharth Date: Sun, 24 May 2026 11:33:23 +0530 Subject: [PATCH 2/4] fix(scoring): apply mode multipliers in verify scoring + fix inline comment severity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - calculate_verify_score() now accepts review_modes and applies the same severity multipliers (security, migration, etc.) as full reviews, fixing the penalty inconsistency between full review and fix verification runs - Posting loop uses mode-adjusted findings so inline comments show the correct severity (e.g. CRITICAL instead of WARNING for security findings) - Remove CI Gate and Next Steps sections from PR summary comment - Add e2e integration test (full review → fix verification, both posting) - Add fix verification integration test for auto-detect re-push flow --- src/post_findings.py | 33 +- src/pr_scorer.py | 19 +- .../test_e2e_review_then_verify.py | 429 ++++++++++++++++++ tests/integration/test_fix_verification.py | 341 ++++++++++++++ 4 files changed, 796 insertions(+), 26 deletions(-) create mode 100644 tests/integration/test_e2e_review_then_verify.py create mode 100644 tests/integration/test_fix_verification.py diff --git a/src/post_findings.py b/src/post_findings.py index 43d629d..403c8da 100644 --- a/src/post_findings.py +++ b/src/post_findings.py @@ -842,28 +842,7 @@ def _build_summary_markdown( lines.append(f"- ➖ **{fv.cr_id}** — Not relevant: {fv.reason}") lines.append("") - # CI Gate - gate_passed = gate_result.get("passed", True) - gate_icon = "✅" if gate_passed else "🚨" - lines += [ - "## 🚦 CI Gate", - f"{gate_icon} Gate: **{'PASSED' if gate_passed else 'FAILED'}**", - "", - ] - if gate_result.get("reasons"): - for reason in gate_result["reasons"]: - lines.append(f"- {reason}") - lines.append("") - - # Next steps - lines += [ - "## 🚀 Next Steps", - "1. Review the inline comments on specific files", - "2. Address critical and warning items", - "3. Consider implementing suggestions for code quality", - "4. Reply to any comments if you need clarification", - "", - ] + # CI Gate (internal use only — not shown in PR summary) # Overall summary — agent-generated narrative if getattr(findings_file, "summary", None): @@ -1197,12 +1176,16 @@ def run( new_findings = [f for f in capped if f.id not in posted_cr_ids] deduped_count = len(capped) - len(new_findings) - # 8. Score + # 8. Score (apply mode multipliers so severity is consistent for scoring AND posting) + all_adjusted = capped if is_verify_only and findings_file.fix_verifications: - score = scorer.calculate_verify_score(findings_file.fix_verifications) + score = scorer.calculate_verify_score(findings_file.fix_verifications, findings_file.review_modes) else: all_adjusted = scorer.apply_mode_multipliers(capped, findings_file.review_modes) score = scorer.calculate_pr_score(all_adjusted) + # Rebuild new_findings from adjusted list so posted comments reflect adjusted severity + adjusted_by_id = {f.id: f for f in all_adjusted} + new_findings = [adjusted_by_id[f.id] for f in new_findings if f.id in adjusted_by_id] # 9. Post inline comments (skip entirely for verify-only) posted_count = 0 @@ -1292,7 +1275,7 @@ def run( # 13. Post/update summary summary_md = _build_summary_markdown( findings_file=findings_file, - filtered_findings=capped, + filtered_findings=all_adjusted if not is_verify_only else capped, score=score, gate_result=gate_result, fix_verifications=findings_file.fix_verifications, diff --git a/src/pr_scorer.py b/src/pr_scorer.py index 99b7a11..e4339e4 100644 --- a/src/pr_scorer.py +++ b/src/pr_scorer.py @@ -259,11 +259,12 @@ def _extract_severity_counts(self, statistics: Dict[str, int]) -> Dict[str, int] 'good': statistics.get('good', 0) } - def calculate_verify_score(self, fix_verifications) -> PRScore: + def calculate_verify_score(self, fix_verifications, review_modes: List[str] = None) -> PRScore: """Calculate score for verify-only runs from prior findings' severity/category. Penalty = sum of penalties for all prior findings that are still_present. Fixed findings contribute zero penalty. + Applies the same mode multipliers as full reviews for consistency. """ if not self.enable_scoring: return self._create_disabled_score() @@ -273,9 +274,25 @@ def calculate_verify_score(self, fix_verifications) -> PRScore: category_penalties: Dict[str, float] = {} stats: Dict[str, int] = {'critical': 0, 'warning': 0, 'suggestion': 0, 'good': 0} + modes = {m.lower() for m in (review_modes or [])} + for fv in fix_verifications: sev = fv.severity or "warning" cat = fv.category or "best_practices" + + # Apply the same mode multipliers used during full review + if 'migration' in modes: + sev = 'critical' + elif 'security' in modes and cat == 'security': + if sev == 'warning': + sev = 'critical' + elif 'performance' in modes and cat == 'performance': + if sev == 'warning': + sev = 'critical' + elif 'architecture' in modes and cat in ('best_practices', 'architecture'): + if sev == 'suggestion': + sev = 'warning' + issue_penalty = self._calculate_issue_penalty(sev, cat) original_penalty += issue_penalty diff --git a/tests/integration/test_e2e_review_then_verify.py b/tests/integration/test_e2e_review_then_verify.py new file mode 100644 index 0000000..dc95310 --- /dev/null +++ b/tests/integration/test_e2e_review_then_verify.py @@ -0,0 +1,429 @@ +""" +End-to-end integration test: full review → fix verification, both posting to PR. + +Phase 1: Run a full code review (NOT dry-run) against the target PR. + Posts inline findings as ADO comment threads. +Phase 2: Re-run the pipeline in FULL mode. Because Phase 1 left CodeHawk + comments (cr-id markers), the pipeline auto-detects a re-push and + switches to verify-only mode. This also posts results to the PR. + +Target: PR 6697 in BluSKYFunctionApps (blub0x / BluSKY Git) + +Run: + pytest tests/integration/test_e2e_review_then_verify.py -v -m integration -s +""" + +import json +import logging +import os +import subprocess +from datetime import datetime +from pathlib import Path + +import pytest +from dotenv import load_dotenv + +from batch_review_job import BatchReviewJob +from config import Settings, reset_settings +from models.review_models import ReviewMode + +from .conftest import ( + PROJECT_ROOT, REVIEW_PROMPT, MAX_TURNS_INTEGRATION, + integration, needs_ado, + save_findings_artifact, log_phase2_summary, +) + +load_dotenv(PROJECT_ROOT / ".env") + +_log = logging.getLogger(__name__) + +# --------------------------------------------------------------------------- +# Test-specific constants +# --------------------------------------------------------------------------- + +E2E_PR_ID = 6698 +E2E_REPO = "BluSKYFunctionApps" +E2E_ADO_ORG = "blub0x" +E2E_ADO_PROJECT = "BluSKY Git" + +RESULTS_DIR = PROJECT_ROOT / "results" + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +def _setup_env() -> Settings: + env = { + "VCS": "ado", + "AZURE_DEVOPS_ORG": E2E_ADO_ORG, + "AZURE_DEVOPS_PROJECT": E2E_ADO_PROJECT, + "AZURE_DEVOPS_PAT": os.environ.get("ADO_PAT", ""), + "AZURE_DEVOPS_REPO": E2E_REPO, + "AUTH_MODE": "pat", + "LOG_LEVEL": "DEBUG", + "LOG_FORMAT": "text", + "ENABLE_GRAPH": "true", + } + for k, v in env.items(): + os.environ[k] = v + reset_settings() + return Settings() + + +def _clone_workspace() -> tuple[Path, str]: + settings = _setup_env() + + from activities.fetch_pr_details_activity import FetchPRDetailsActivity + from models.review_models import FetchPRDetailsInput + + activity = FetchPRDetailsActivity(settings=settings) + pr = activity.execute(FetchPRDetailsInput(pr_id=E2E_PR_ID, repository_id=E2E_REPO)) + source_branch = pr.source_branch + + workspace = RESULTS_DIR / "workspace-e2e" + pat = os.environ["AZURE_DEVOPS_PAT"] + project_encoded = E2E_ADO_PROJECT.replace(" ", "%20") + repo_encoded = E2E_REPO.replace(" ", "%20") + auth_url = f"https://{pat}@dev.azure.com/{E2E_ADO_ORG}/{project_encoded}/_git/{repo_encoded}" + + if (workspace / ".git").exists(): + _log.info("Re-using existing clone at %s", workspace) + subprocess.run( + ["git", "remote", "set-url", "origin", auth_url], + cwd=str(workspace), check=True, capture_output=True, text=True, timeout=10, + ) + subprocess.run( + ["git", "config", "remote.origin.fetch", "+refs/heads/*:refs/remotes/origin/*"], + cwd=str(workspace), check=True, capture_output=True, text=True, timeout=10, + ) + subprocess.run( + ["git", "fetch", "origin", source_branch, "--depth", "1"], + cwd=str(workspace), check=True, capture_output=True, text=True, timeout=120, + ) + subprocess.run( + ["git", "checkout", "-B", source_branch, f"origin/{source_branch}"], + cwd=str(workspace), check=True, capture_output=True, text=True, timeout=30, + ) + else: + _log.info("Cloning %s (branch: %s)...", E2E_REPO, source_branch) + workspace.parent.mkdir(parents=True, exist_ok=True) + subprocess.run( + ["git", "clone", "--branch", source_branch, "--depth", "1", + auth_url, str(workspace)], + check=True, capture_output=True, text=True, timeout=600, + ) + + (workspace / ".cr").mkdir(exist_ok=True) + return workspace, source_branch + + +def _setup_file_logging(label: str) -> tuple[Path, logging.FileHandler]: + RESULTS_DIR.mkdir(exist_ok=True) + timestamp = datetime.now().strftime("%Y-%m-%d_%H%M%S") + log_path = RESULTS_DIR / f"e2e-{label}-{E2E_PR_ID}-{timestamp}.log" + + handler = logging.FileHandler(str(log_path), mode="w", encoding="utf-8") + handler.setLevel(logging.DEBUG) + handler.setFormatter(logging.Formatter( + "%(asctime)s [%(levelname)s] %(name)s — %(message)s", + datefmt="%Y-%m-%d %H:%M:%S", + )) + + root = logging.getLogger("codehawk") + root.addHandler(handler) + root.setLevel(logging.DEBUG) + + console = logging.StreamHandler() + console.setLevel(logging.INFO) + console.setFormatter(logging.Formatter( + "%(asctime)s [%(levelname)s] %(name)s — %(message)s", + datefmt="%H:%M:%S", + )) + if not any(isinstance(h, logging.StreamHandler) and not isinstance(h, logging.FileHandler) + for h in root.handlers): + root.addHandler(console) + + _log.info("Logging to: %s", log_path) + return log_path, handler + + +def _teardown_file_logging(handler: logging.FileHandler): + logging.getLogger("codehawk").removeHandler(handler) + handler.close() + + +def _count_codehawk_comments(settings, pr_id, repo) -> int: + from activities.fetch_pr_comments_activity import FetchPRCommentsActivity + + activity = FetchPRCommentsActivity(settings=settings) + all_threads = activity.execute(pr_id=pr_id, repository_id=repo) + codehawk_threads = [t for t in all_threads if t.cr_id] + _log.info( + "CodeHawk comments: %d total threads, %d with cr-id markers", + len(all_threads), len(codehawk_threads), + ) + return len(codehawk_threads) + + +def _save_run_log(label: str, data: dict): + RESULTS_DIR.mkdir(exist_ok=True) + timestamp = datetime.now().strftime("%Y-%m-%d_%H%M%S") + log_file = RESULTS_DIR / f"e2e-{label}-{E2E_PR_ID}-{timestamp}.json" + log_file.write_text(json.dumps(data, indent=2), encoding="utf-8") + _log.info("Run log saved: %s", log_file) + + +# --------------------------------------------------------------------------- +# Phase 1: Full review (posts comments to PR) +# --------------------------------------------------------------------------- + +@integration +@needs_ado +class TestE2EFullReview: + """Phase 1 — full code review, dry_run=False, posts findings to the PR.""" + + @pytest.fixture(autouse=True, scope="class") + def pipeline_result(self, request): + log_path, log_handler = _setup_file_logging("full-review") + + settings = _setup_env() + workspace, source_branch = _clone_workspace() + _log.info("Phase 1 — Full review on branch: %s", source_branch) + + model = os.environ.get("OPENAI_MODEL", "gpt-4o-mini") + + job = BatchReviewJob( + pr_id=E2E_PR_ID, + repo=E2E_REPO, + workspace=workspace, + model=model, + prompt_path=REVIEW_PROMPT, + settings=settings, + review_mode=ReviewMode.FULL, + ) + + try: + phase2_error = None + phase2_output = None + findings_path = workspace / ".cr" / "findings.json" + + try: + phase2_output = job.run(dry_run=False) + except SystemExit as exc: + _log.warning("Quality gate exit code %s", exc.code) + phase2_error = exc + except Exception as exc: + _log.error("Pipeline error: %s", exc) + phase2_error = exc + + if findings_path.exists(): + findings_data = json.loads(findings_path.read_text(encoding="utf-8")) + else: + findings_data = {"findings": []} + + save_findings_artifact(findings_data, "e2e-full-review") + + if phase2_output: + log_phase2_summary(phase2_output) + + _save_run_log("full-review", { + "run_date": datetime.now().isoformat(), + "pr_id": E2E_PR_ID, + "repo": E2E_REPO, + "mode": "full-review (dry_run=False)", + "findings": findings_data, + "phase2": phase2_output, + "error": str(phase2_error) if phase2_error else None, + }) + + request.cls.findings_data = findings_data + request.cls.phase2_output = phase2_output + request.cls.phase2_error = phase2_error + request.cls.workspace = workspace + finally: + _teardown_file_logging(log_handler) + _log.info("Phase 1 log saved: %s", log_path) + + # ------------------------------------------------------------------ + # Phase 1 assertions + # ------------------------------------------------------------------ + + def test_pipeline_ran(self): + assert self.findings_data is not None, "Pipeline did not produce findings" + + def test_findings_produced(self): + findings = self.findings_data.get("findings", []) + _log.info("Phase 1 produced %d findings", len(findings)) + assert len(findings) > 0, "Full review should produce at least one finding" + + def test_findings_have_required_fields(self): + for f in self.findings_data.get("findings", []): + assert f.get("file"), f"Finding missing 'file': {f}" + assert f.get("title"), f"Finding missing 'title': {f}" + assert f.get("severity"), f"Finding missing 'severity': {f}" + + def test_phase2_ran(self): + assert self.phase2_output is not None or self.phase2_error is not None, ( + "Neither phase2 output nor error — pipeline may not have run" + ) + + def test_comments_posted_to_pr(self): + """After dry_run=False, the PR should have CodeHawk comment threads.""" + settings = _setup_env() + count = _count_codehawk_comments(settings, E2E_PR_ID, E2E_REPO) + _log.info("Post-review cr-id comment count: %d", count) + assert count > 0, ( + "No CodeHawk comments found on PR after full review with dry_run=False" + ) + + +# --------------------------------------------------------------------------- +# Phase 2: Fix verification (posts results to PR) +# --------------------------------------------------------------------------- + +@integration +@needs_ado +class TestE2EFixVerification: + """Phase 2 — re-run pipeline; auto-detects prior comments, runs verify-only, posts results.""" + + @pytest.fixture(autouse=True, scope="class") + def pipeline_result(self, request): + log_path, log_handler = _setup_file_logging("fix-verify") + + settings = _setup_env() + workspace, source_branch = _clone_workspace() + _log.info("Phase 2 — Fix verification on branch: %s", source_branch) + + prior_count = _count_codehawk_comments(settings, E2E_PR_ID, E2E_REPO) + if prior_count == 0: + pytest.skip( + f"PR {E2E_PR_ID} has no existing CodeHawk comments — " + "run TestE2EFullReview first to create them." + ) + _log.info("PR has %d prior CodeHawk comments — should auto-detect re-push", prior_count) + + model = os.environ.get("OPENAI_MODEL", "gpt-4o-mini") + + job = BatchReviewJob( + pr_id=E2E_PR_ID, + repo=E2E_REPO, + workspace=workspace, + model=model, + prompt_path=REVIEW_PROMPT, + settings=settings, + review_mode=ReviewMode.FULL, + ) + + try: + phase2_error = None + phase2_output = None + findings_path = workspace / ".cr" / "findings.json" + + try: + phase2_output = job.run(dry_run=False) + except SystemExit as exc: + _log.warning("Quality gate exit code %s", exc.code) + phase2_error = exc + except Exception as exc: + _log.error("Pipeline error: %s", exc) + phase2_error = exc + + if findings_path.exists(): + findings_data = json.loads(findings_path.read_text(encoding="utf-8")) + else: + findings_data = {"findings": [], "fix_verifications": []} + + save_findings_artifact(findings_data, "e2e-fix-verify") + + _log.info("fix_verifications count: %d", len(findings_data.get("fix_verifications", []))) + for fv in findings_data.get("fix_verifications", []): + _log.info(" %s → %s: %s", fv.get("cr_id"), fv.get("status"), fv.get("reason", "")[:100]) + + if phase2_output: + log_phase2_summary(phase2_output) + + _save_run_log("fix-verify", { + "run_date": datetime.now().isoformat(), + "pr_id": E2E_PR_ID, + "repo": E2E_REPO, + "mode": "fix-verify (auto-detect, dry_run=False)", + "prior_codehawk_comments": prior_count, + "findings": findings_data, + "phase2": phase2_output, + "error": str(phase2_error) if phase2_error else None, + }) + + request.cls.findings_data = findings_data + request.cls.phase2_output = phase2_output + request.cls.phase2_error = phase2_error + request.cls.prior_count = prior_count + request.cls.workspace = workspace + finally: + _teardown_file_logging(log_handler) + _log.info("Phase 2 log saved: %s", log_path) + + # ------------------------------------------------------------------ + # Phase 2 assertions + # ------------------------------------------------------------------ + + def test_pipeline_ran(self): + assert self.findings_data is not None, "Pipeline did not produce findings" + + def test_no_new_findings(self): + findings = self.findings_data.get("findings", []) + assert findings == [], ( + f"Verify-only should produce 0 new findings, got {len(findings)}" + ) + + def test_verify_fixes_mode_set(self): + modes = self.findings_data.get("review_modes", []) + assert "verify_fixes" in modes, f"Expected 'verify_fixes' in review_modes, got {modes}" + + def test_fix_verifications_present(self): + fv_list = self.findings_data.get("fix_verifications", []) + assert len(fv_list) > 0, ( + f"Expected fix_verifications to be non-empty — pipeline should have " + f"auto-detected {self.prior_count} prior CodeHawk comments" + ) + + def test_fix_verifications_cover_prior_findings(self): + fv_list = self.findings_data.get("fix_verifications", []) + assert len(fv_list) >= self.prior_count * 0.5, ( + f"Only {len(fv_list)} fix_verifications for {self.prior_count} prior comments — " + f"expected at least 50% coverage" + ) + + def test_fix_verification_statuses_valid(self): + valid_statuses = {"fixed", "still_present", "not_relevant"} + for fv in self.findings_data.get("fix_verifications", []): + assert fv.get("status") in valid_statuses, ( + f"Invalid status '{fv.get('status')}' for {fv.get('cr_id')}" + ) + + def test_fix_verifications_have_reasons(self): + for fv in self.findings_data.get("fix_verifications", []): + reason = fv.get("reason", "") + assert reason and len(reason) > 5, ( + f"Missing or too-short reason for {fv.get('cr_id')}: '{reason}'" + ) + + def test_phase2_output_includes_fix_verifications(self): + if self.phase2_output is None: + pytest.skip("Phase 2 failed — cannot check output structure") + fv_out = self.phase2_output.get("fix_verifications", []) + assert len(fv_out) > 0, "Phase 2 output should include fix_verifications" + + def test_phase2_has_comparison_flag(self): + if self.phase2_output is None: + pytest.skip("Phase 2 failed — cannot check output structure") + assert "has_comparison" in self.phase2_output, ( + "Phase 2 output missing 'has_comparison' key" + ) + + def test_verify_comments_posted_to_pr(self): + """After fix verification with dry_run=False, PR should still have CodeHawk threads.""" + settings = _setup_env() + count = _count_codehawk_comments(settings, E2E_PR_ID, E2E_REPO) + _log.info("Post-verify cr-id comment count: %d", count) + assert count > 0, "Expected CodeHawk comments on PR after fix verification" diff --git a/tests/integration/test_fix_verification.py b/tests/integration/test_fix_verification.py new file mode 100644 index 0000000..d06a36f --- /dev/null +++ b/tests/integration/test_fix_verification.py @@ -0,0 +1,341 @@ +""" +Fix verification integration test — re-push flow (auto-detect). + +Runs the pipeline in default FULL mode (no flags) against a PR that already +has CodeHawk comments. The pipeline should auto-detect the re-push, fetch +prior findings, force single-pass, and produce fix_verifications[]. + +This mirrors how CI works in production — no --verify-fixes flag needed. + +Target: PR 6658 in BluSKY repo (blub0x / BluSKY Git) + +Prerequisites: + - The target PR must have existing CodeHawk comments (with markers) + - ADO_PAT and OPENAI_API_KEY must be set (via .env or environment) + +Run: + pytest tests/integration/test_fix_verification.py -v -m integration -s +""" + +import json +import logging +import os +import subprocess +from datetime import datetime +from pathlib import Path + +import pytest +from dotenv import load_dotenv + +from batch_review_job import BatchReviewJob +from config import Settings, reset_settings +from models.review_models import ReviewMode + +from .conftest import ( + PROJECT_ROOT, REVIEW_PROMPT, MAX_TURNS_INTEGRATION, + integration, needs_ado, + save_findings_artifact, log_phase2_summary, +) + +load_dotenv(PROJECT_ROOT / ".env") + +_log = logging.getLogger(__name__) + +# --------------------------------------------------------------------------- +# Test-specific constants — PR 6658 in BluSKY +# --------------------------------------------------------------------------- + +FV_PR_ID = 6658 +FV_REPO = "BluSKY" +FV_ADO_ORG = "blub0x" +FV_ADO_PROJECT = "BluSKY Git" + +RESULTS_DIR = PROJECT_ROOT / "results" + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +def _setup_fv_env() -> Settings: + """Configure env vars for BluSKY repo and return fresh Settings.""" + env = { + "VCS": "ado", + "AZURE_DEVOPS_ORG": FV_ADO_ORG, + "AZURE_DEVOPS_PROJECT": FV_ADO_PROJECT, + "AZURE_DEVOPS_PAT": os.environ.get("ADO_PAT", ""), + "AZURE_DEVOPS_REPO": FV_REPO, + "AUTH_MODE": "pat", + "LOG_LEVEL": "DEBUG", + "LOG_FORMAT": "text", + "ENABLE_GRAPH": "true", + } + for k, v in env.items(): + os.environ[k] = v + reset_settings() + return Settings() + + +def _clone_fv_workspace() -> tuple[Path, str]: + """Clone the BluSKY repo and checkout the PR 6658 source branch.""" + settings = _setup_fv_env() + + from activities.fetch_pr_details_activity import FetchPRDetailsActivity + from models.review_models import FetchPRDetailsInput + + activity = FetchPRDetailsActivity(settings=settings) + pr = activity.execute(FetchPRDetailsInput(pr_id=FV_PR_ID, repository_id=FV_REPO)) + source_branch = pr.source_branch + + workspace = RESULTS_DIR / "workspace-blusky" + pat = os.environ["AZURE_DEVOPS_PAT"] + project_encoded = FV_ADO_PROJECT.replace(" ", "%20") + repo_encoded = FV_REPO.replace(" ", "%20") + auth_url = f"https://{pat}@dev.azure.com/{FV_ADO_ORG}/{project_encoded}/_git/{repo_encoded}" + + if (workspace / ".git").exists(): + _log.info("Re-using existing clone at %s", workspace) + subprocess.run( + ["git", "remote", "set-url", "origin", auth_url], + cwd=str(workspace), check=True, capture_output=True, text=True, timeout=10, + ) + subprocess.run( + ["git", "config", "remote.origin.fetch", "+refs/heads/*:refs/remotes/origin/*"], + cwd=str(workspace), check=True, capture_output=True, text=True, timeout=10, + ) + subprocess.run( + ["git", "fetch", "origin", source_branch, "--depth", "1"], + cwd=str(workspace), check=True, capture_output=True, text=True, timeout=120, + ) + subprocess.run( + ["git", "checkout", "-B", source_branch, f"origin/{source_branch}"], + cwd=str(workspace), check=True, capture_output=True, text=True, timeout=30, + ) + else: + _log.info("Cloning %s (branch: %s)...", FV_REPO, source_branch) + workspace.parent.mkdir(parents=True, exist_ok=True) + subprocess.run( + ["git", "clone", "--branch", source_branch, "--depth", "1", + auth_url, str(workspace)], + check=True, capture_output=True, text=True, timeout=600, + ) + + (workspace / ".cr").mkdir(exist_ok=True) + return workspace, source_branch + + +def _setup_file_logging() -> tuple[Path, logging.FileHandler]: + RESULTS_DIR.mkdir(exist_ok=True) + timestamp = datetime.now().strftime("%Y-%m-%d_%H%M%S") + log_path = RESULTS_DIR / f"fix-verify-{FV_PR_ID}-{timestamp}.log" + + handler = logging.FileHandler(str(log_path), mode="w", encoding="utf-8") + handler.setLevel(logging.DEBUG) + handler.setFormatter(logging.Formatter( + "%(asctime)s [%(levelname)s] %(name)s — %(message)s", + datefmt="%Y-%m-%d %H:%M:%S", + )) + + root = logging.getLogger("codehawk") + root.addHandler(handler) + root.setLevel(logging.DEBUG) + + console = logging.StreamHandler() + console.setLevel(logging.INFO) + console.setFormatter(logging.Formatter( + "%(asctime)s [%(levelname)s] %(name)s — %(message)s", + datefmt="%H:%M:%S", + )) + if not any(isinstance(h, logging.StreamHandler) and not isinstance(h, logging.FileHandler) + for h in root.handlers): + root.addHandler(console) + + _log.info("Logging to: %s", log_path) + return log_path, handler + + +def _teardown_file_logging(handler: logging.FileHandler): + logging.getLogger("codehawk").removeHandler(handler) + handler.close() + + +def _count_codehawk_comments(settings, pr_id, repo) -> int: + """Quick check: how many CodeHawk comment threads exist on this PR?""" + from activities.fetch_pr_comments_activity import FetchPRCommentsActivity + + activity = FetchPRCommentsActivity(settings=settings) + all_threads = activity.execute(pr_id=pr_id, repository_id=repo) + codehawk_threads = [t for t in all_threads if t.cr_id] + _log.info( + "Pre-check: %d total threads, %d with cr-id markers", + len(all_threads), len(codehawk_threads), + ) + for t in codehawk_threads: + _log.info( + " %s: %s (line %d in %s)", + t.cr_id, t.comment_text[:80].replace("\n", " "), + t.line_number, t.file_path, + ) + return len(codehawk_threads) + + +# --------------------------------------------------------------------------- +# Test class +# --------------------------------------------------------------------------- + +@integration +@needs_ado +class TestFixVerification: + """Run pipeline in default FULL mode — auto-detects re-push from existing PR comments.""" + + @pytest.fixture(autouse=True, scope="class") + def pipeline_result(self, request): + log_path, log_handler = _setup_file_logging() + + settings = _setup_fv_env() + workspace, source_branch = _clone_fv_workspace() + _log.info("Branch: %s", source_branch) + + # Pre-check: PR must have existing CodeHawk comments for this test to be meaningful + prior_count = _count_codehawk_comments(settings, FV_PR_ID, FV_REPO) + if prior_count == 0: + pytest.skip( + f"PR {FV_PR_ID} has no existing CodeHawk comments (no cr-id markers) — " + "cannot test fix verification. Run a normal review first to create comments." + ) + _log.info("PR has %d prior CodeHawk comments — pipeline should auto-detect re-push", prior_count) + + model = os.environ.get("OPENAI_MODEL", "gpt-4o-mini") + + # Use BatchReviewJob with default FULL mode — no --verify-fixes flag. + # The pipeline should auto-detect prior findings and handle them. + job = BatchReviewJob( + pr_id=FV_PR_ID, + repo=FV_REPO, + workspace=workspace, + model=model, + prompt_path=REVIEW_PROMPT, + settings=settings, + review_mode=ReviewMode.FULL, + ) + + try: + phase2_error = None + phase2_output = None + findings_path = workspace / ".cr" / "findings.json" + + try: + phase2_output = job.run(dry_run=True) + except SystemExit as exc: + _log.warning("Quality gate exit code %s", exc.code) + phase2_error = exc + except Exception as exc: + _log.error("Pipeline error: %s", exc) + phase2_error = exc + + if findings_path.exists(): + findings_data = json.loads(findings_path.read_text(encoding="utf-8")) + else: + findings_data = {"findings": [], "fix_verifications": []} + + save_findings_artifact(findings_data, "fix-verify") + + _log.info("fix_verifications count: %d", len(findings_data.get("fix_verifications", []))) + for fv in findings_data.get("fix_verifications", []): + _log.info(" %s → %s: %s", fv.get("cr_id"), fv.get("status"), fv.get("reason", "")[:100]) + + if phase2_output: + log_phase2_summary(phase2_output) + + # Save run log + RESULTS_DIR.mkdir(exist_ok=True) + timestamp = datetime.now().strftime("%Y-%m-%d_%H%M%S") + run_log = { + "run_date": datetime.now().isoformat(), + "pr_id": FV_PR_ID, + "repo": FV_REPO, + "mode": "verify-only (auto-detect re-push)", + "prior_codehawk_comments": prior_count, + "findings": findings_data, + "phase2": phase2_output, + "error": str(phase2_error) if phase2_error else None, + } + log_file = RESULTS_DIR / f"fix-verify-{FV_PR_ID}-{timestamp}.json" + log_file.write_text(json.dumps(run_log, indent=2), encoding="utf-8") + _log.info("Run log saved: %s", log_file) + + request.cls.findings_data = findings_data + request.cls.phase2_output = phase2_output + request.cls.phase2_error = phase2_error + request.cls.prior_count = prior_count + request.cls.workspace = workspace + finally: + _teardown_file_logging(log_handler) + _log.info("Full log saved: %s", log_path) + + # ------------------------------------------------------------------ + # Assertions + # ------------------------------------------------------------------ + + def test_pipeline_ran(self): + """Pipeline must produce findings.json.""" + assert self.findings_data is not None, "Pipeline did not produce findings" + + def test_no_new_findings(self): + """Re-push verify-only mode should skip full review — no new findings.""" + findings = self.findings_data.get("findings", []) + assert findings == [], ( + f"Verify-only should produce 0 new findings, got {len(findings)}" + ) + + def test_verify_fixes_mode_set(self): + """findings.json should have verify_fixes in review_modes.""" + modes = self.findings_data.get("review_modes", []) + assert "verify_fixes" in modes, f"Expected 'verify_fixes' in review_modes, got {modes}" + + def test_fix_verifications_present(self): + """Auto-detected re-push must produce fix_verifications.""" + fv_list = self.findings_data.get("fix_verifications", []) + assert len(fv_list) > 0, ( + f"Expected fix_verifications to be non-empty — pipeline should have " + f"auto-detected {self.prior_count} prior CodeHawk comments" + ) + + def test_fix_verifications_cover_prior_findings(self): + """fix_verifications should cover a reasonable portion of prior cr-ids.""" + fv_list = self.findings_data.get("fix_verifications", []) + assert len(fv_list) >= self.prior_count * 0.5, ( + f"Only {len(fv_list)} fix_verifications for {self.prior_count} prior comments — " + f"expected at least 50% coverage" + ) + + def test_fix_verification_statuses_valid(self): + """Each fix_verification must have a valid status.""" + valid_statuses = {"fixed", "still_present", "not_relevant"} + for fv in self.findings_data.get("fix_verifications", []): + assert fv.get("status") in valid_statuses, ( + f"Invalid status '{fv.get('status')}' for {fv.get('cr_id')}" + ) + + def test_fix_verifications_have_reasons(self): + """Each fix_verification should include a reason.""" + for fv in self.findings_data.get("fix_verifications", []): + reason = fv.get("reason", "") + assert reason and len(reason) > 5, ( + f"Missing or too-short reason for {fv.get('cr_id')}: '{reason}'" + ) + + def test_phase2_output_includes_fix_verifications(self): + """Phase 2 structured output should include fix_verifications.""" + if self.phase2_output is None: + pytest.skip("Phase 2 failed — cannot check output structure") + fv_out = self.phase2_output.get("fix_verifications", []) + assert len(fv_out) > 0, "Phase 2 output should include fix_verifications" + + def test_phase2_has_comparison_flag(self): + """Phase 2 output should set has_comparison when fix_verifications are present.""" + if self.phase2_output is None: + pytest.skip("Phase 2 failed — cannot check output structure") + assert "has_comparison" in self.phase2_output, ( + "Phase 2 output missing 'has_comparison' key" + ) From 9bc29126e91362032ab23302cac6e4c038780443 Mon Sep 17 00:00:00 2001 From: Siddharth Date: Sun, 24 May 2026 11:54:42 +0530 Subject: [PATCH 3/4] chore: remove org-specific references from public repo - Gitignore live integration tests (test_e2e_review_then_verify, test_fix_verification) that contain org-specific ADO config - Replace hardcoded org/repo/PR values in conftest.py with env vars - Replace hardcoded repo name in test_phase2_scoring.py with generic - Scrub company name from workspace_tools.py comment --- .gitignore | 4 + src/tools/workspace_tools.py | 2 +- tests/integration/conftest.py | 22 +- .../test_e2e_review_then_verify.py | 429 ------------------ tests/integration/test_fix_verification.py | 341 -------------- tests/unit/test_phase2_scoring.py | 4 +- 6 files changed, 11 insertions(+), 791 deletions(-) delete mode 100644 tests/integration/test_e2e_review_then_verify.py delete mode 100644 tests/integration/test_fix_verification.py diff --git a/.gitignore b/.gitignore index 4a698f7..289fc84 100644 --- a/.gitignore +++ b/.gitignore @@ -41,6 +41,10 @@ coverage/ results/ findings-output/ +# Live integration tests (contain org-specific config) +tests/integration/test_e2e_review_then_verify.py +tests/integration/test_fix_verification.py + # Python __pycache__/ *.pyc diff --git a/src/tools/workspace_tools.py b/src/tools/workspace_tools.py index 9d6ad1d..1a6125c 100644 --- a/src/tools/workspace_tools.py +++ b/src/tools/workspace_tools.py @@ -15,7 +15,7 @@ def _resolve_workspace_path(workspace: Path, file_path: str) -> Path: """Resolve a file path safely within the workspace. - ADO file paths start with '/' (e.g. '/BluSKYFunctionApps/...'). + ADO file paths start with '/' (e.g. '/MyRepo/...'). On Windows, Path(workspace) / '/absolute' drops the workspace prefix. Strip the leading '/' so it joins correctly. """ diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 798fea7..d302d2d 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -29,24 +29,10 @@ MAX_TURNS_INTEGRATION = 40 -# Large PR -#PR_ID = 6435 -#PR_ID = 6619 - -#Small PR -#PR_ID = 6629 -# REPO = "BluSKYFunctionApps" -# ADO_ORG = "blub0x" -# ADO_PROJECT = "BluSKY Git" - -#REPO = "AI Pipelines" -#ADO_ORG = "blub0x" -#ADO_PROJECT = "BluB0X AI" - -PR_ID = 6686 -REPO = "BluSKYFunctionApps" -ADO_ORG = "blub0x" -ADO_PROJECT = "BluSKY Git" +PR_ID = int(os.environ.get("TEST_PR_ID", "1")) +REPO = os.environ.get("TEST_REPO", "MyRepo") +ADO_ORG = os.environ.get("TEST_ADO_ORG", "my-org") +ADO_PROJECT = os.environ.get("TEST_ADO_PROJECT", "My Project") PROJECT_ROOT = Path(__file__).parent.parent.parent diff --git a/tests/integration/test_e2e_review_then_verify.py b/tests/integration/test_e2e_review_then_verify.py deleted file mode 100644 index dc95310..0000000 --- a/tests/integration/test_e2e_review_then_verify.py +++ /dev/null @@ -1,429 +0,0 @@ -""" -End-to-end integration test: full review → fix verification, both posting to PR. - -Phase 1: Run a full code review (NOT dry-run) against the target PR. - Posts inline findings as ADO comment threads. -Phase 2: Re-run the pipeline in FULL mode. Because Phase 1 left CodeHawk - comments (cr-id markers), the pipeline auto-detects a re-push and - switches to verify-only mode. This also posts results to the PR. - -Target: PR 6697 in BluSKYFunctionApps (blub0x / BluSKY Git) - -Run: - pytest tests/integration/test_e2e_review_then_verify.py -v -m integration -s -""" - -import json -import logging -import os -import subprocess -from datetime import datetime -from pathlib import Path - -import pytest -from dotenv import load_dotenv - -from batch_review_job import BatchReviewJob -from config import Settings, reset_settings -from models.review_models import ReviewMode - -from .conftest import ( - PROJECT_ROOT, REVIEW_PROMPT, MAX_TURNS_INTEGRATION, - integration, needs_ado, - save_findings_artifact, log_phase2_summary, -) - -load_dotenv(PROJECT_ROOT / ".env") - -_log = logging.getLogger(__name__) - -# --------------------------------------------------------------------------- -# Test-specific constants -# --------------------------------------------------------------------------- - -E2E_PR_ID = 6698 -E2E_REPO = "BluSKYFunctionApps" -E2E_ADO_ORG = "blub0x" -E2E_ADO_PROJECT = "BluSKY Git" - -RESULTS_DIR = PROJECT_ROOT / "results" - - -# --------------------------------------------------------------------------- -# Helpers -# --------------------------------------------------------------------------- - -def _setup_env() -> Settings: - env = { - "VCS": "ado", - "AZURE_DEVOPS_ORG": E2E_ADO_ORG, - "AZURE_DEVOPS_PROJECT": E2E_ADO_PROJECT, - "AZURE_DEVOPS_PAT": os.environ.get("ADO_PAT", ""), - "AZURE_DEVOPS_REPO": E2E_REPO, - "AUTH_MODE": "pat", - "LOG_LEVEL": "DEBUG", - "LOG_FORMAT": "text", - "ENABLE_GRAPH": "true", - } - for k, v in env.items(): - os.environ[k] = v - reset_settings() - return Settings() - - -def _clone_workspace() -> tuple[Path, str]: - settings = _setup_env() - - from activities.fetch_pr_details_activity import FetchPRDetailsActivity - from models.review_models import FetchPRDetailsInput - - activity = FetchPRDetailsActivity(settings=settings) - pr = activity.execute(FetchPRDetailsInput(pr_id=E2E_PR_ID, repository_id=E2E_REPO)) - source_branch = pr.source_branch - - workspace = RESULTS_DIR / "workspace-e2e" - pat = os.environ["AZURE_DEVOPS_PAT"] - project_encoded = E2E_ADO_PROJECT.replace(" ", "%20") - repo_encoded = E2E_REPO.replace(" ", "%20") - auth_url = f"https://{pat}@dev.azure.com/{E2E_ADO_ORG}/{project_encoded}/_git/{repo_encoded}" - - if (workspace / ".git").exists(): - _log.info("Re-using existing clone at %s", workspace) - subprocess.run( - ["git", "remote", "set-url", "origin", auth_url], - cwd=str(workspace), check=True, capture_output=True, text=True, timeout=10, - ) - subprocess.run( - ["git", "config", "remote.origin.fetch", "+refs/heads/*:refs/remotes/origin/*"], - cwd=str(workspace), check=True, capture_output=True, text=True, timeout=10, - ) - subprocess.run( - ["git", "fetch", "origin", source_branch, "--depth", "1"], - cwd=str(workspace), check=True, capture_output=True, text=True, timeout=120, - ) - subprocess.run( - ["git", "checkout", "-B", source_branch, f"origin/{source_branch}"], - cwd=str(workspace), check=True, capture_output=True, text=True, timeout=30, - ) - else: - _log.info("Cloning %s (branch: %s)...", E2E_REPO, source_branch) - workspace.parent.mkdir(parents=True, exist_ok=True) - subprocess.run( - ["git", "clone", "--branch", source_branch, "--depth", "1", - auth_url, str(workspace)], - check=True, capture_output=True, text=True, timeout=600, - ) - - (workspace / ".cr").mkdir(exist_ok=True) - return workspace, source_branch - - -def _setup_file_logging(label: str) -> tuple[Path, logging.FileHandler]: - RESULTS_DIR.mkdir(exist_ok=True) - timestamp = datetime.now().strftime("%Y-%m-%d_%H%M%S") - log_path = RESULTS_DIR / f"e2e-{label}-{E2E_PR_ID}-{timestamp}.log" - - handler = logging.FileHandler(str(log_path), mode="w", encoding="utf-8") - handler.setLevel(logging.DEBUG) - handler.setFormatter(logging.Formatter( - "%(asctime)s [%(levelname)s] %(name)s — %(message)s", - datefmt="%Y-%m-%d %H:%M:%S", - )) - - root = logging.getLogger("codehawk") - root.addHandler(handler) - root.setLevel(logging.DEBUG) - - console = logging.StreamHandler() - console.setLevel(logging.INFO) - console.setFormatter(logging.Formatter( - "%(asctime)s [%(levelname)s] %(name)s — %(message)s", - datefmt="%H:%M:%S", - )) - if not any(isinstance(h, logging.StreamHandler) and not isinstance(h, logging.FileHandler) - for h in root.handlers): - root.addHandler(console) - - _log.info("Logging to: %s", log_path) - return log_path, handler - - -def _teardown_file_logging(handler: logging.FileHandler): - logging.getLogger("codehawk").removeHandler(handler) - handler.close() - - -def _count_codehawk_comments(settings, pr_id, repo) -> int: - from activities.fetch_pr_comments_activity import FetchPRCommentsActivity - - activity = FetchPRCommentsActivity(settings=settings) - all_threads = activity.execute(pr_id=pr_id, repository_id=repo) - codehawk_threads = [t for t in all_threads if t.cr_id] - _log.info( - "CodeHawk comments: %d total threads, %d with cr-id markers", - len(all_threads), len(codehawk_threads), - ) - return len(codehawk_threads) - - -def _save_run_log(label: str, data: dict): - RESULTS_DIR.mkdir(exist_ok=True) - timestamp = datetime.now().strftime("%Y-%m-%d_%H%M%S") - log_file = RESULTS_DIR / f"e2e-{label}-{E2E_PR_ID}-{timestamp}.json" - log_file.write_text(json.dumps(data, indent=2), encoding="utf-8") - _log.info("Run log saved: %s", log_file) - - -# --------------------------------------------------------------------------- -# Phase 1: Full review (posts comments to PR) -# --------------------------------------------------------------------------- - -@integration -@needs_ado -class TestE2EFullReview: - """Phase 1 — full code review, dry_run=False, posts findings to the PR.""" - - @pytest.fixture(autouse=True, scope="class") - def pipeline_result(self, request): - log_path, log_handler = _setup_file_logging("full-review") - - settings = _setup_env() - workspace, source_branch = _clone_workspace() - _log.info("Phase 1 — Full review on branch: %s", source_branch) - - model = os.environ.get("OPENAI_MODEL", "gpt-4o-mini") - - job = BatchReviewJob( - pr_id=E2E_PR_ID, - repo=E2E_REPO, - workspace=workspace, - model=model, - prompt_path=REVIEW_PROMPT, - settings=settings, - review_mode=ReviewMode.FULL, - ) - - try: - phase2_error = None - phase2_output = None - findings_path = workspace / ".cr" / "findings.json" - - try: - phase2_output = job.run(dry_run=False) - except SystemExit as exc: - _log.warning("Quality gate exit code %s", exc.code) - phase2_error = exc - except Exception as exc: - _log.error("Pipeline error: %s", exc) - phase2_error = exc - - if findings_path.exists(): - findings_data = json.loads(findings_path.read_text(encoding="utf-8")) - else: - findings_data = {"findings": []} - - save_findings_artifact(findings_data, "e2e-full-review") - - if phase2_output: - log_phase2_summary(phase2_output) - - _save_run_log("full-review", { - "run_date": datetime.now().isoformat(), - "pr_id": E2E_PR_ID, - "repo": E2E_REPO, - "mode": "full-review (dry_run=False)", - "findings": findings_data, - "phase2": phase2_output, - "error": str(phase2_error) if phase2_error else None, - }) - - request.cls.findings_data = findings_data - request.cls.phase2_output = phase2_output - request.cls.phase2_error = phase2_error - request.cls.workspace = workspace - finally: - _teardown_file_logging(log_handler) - _log.info("Phase 1 log saved: %s", log_path) - - # ------------------------------------------------------------------ - # Phase 1 assertions - # ------------------------------------------------------------------ - - def test_pipeline_ran(self): - assert self.findings_data is not None, "Pipeline did not produce findings" - - def test_findings_produced(self): - findings = self.findings_data.get("findings", []) - _log.info("Phase 1 produced %d findings", len(findings)) - assert len(findings) > 0, "Full review should produce at least one finding" - - def test_findings_have_required_fields(self): - for f in self.findings_data.get("findings", []): - assert f.get("file"), f"Finding missing 'file': {f}" - assert f.get("title"), f"Finding missing 'title': {f}" - assert f.get("severity"), f"Finding missing 'severity': {f}" - - def test_phase2_ran(self): - assert self.phase2_output is not None or self.phase2_error is not None, ( - "Neither phase2 output nor error — pipeline may not have run" - ) - - def test_comments_posted_to_pr(self): - """After dry_run=False, the PR should have CodeHawk comment threads.""" - settings = _setup_env() - count = _count_codehawk_comments(settings, E2E_PR_ID, E2E_REPO) - _log.info("Post-review cr-id comment count: %d", count) - assert count > 0, ( - "No CodeHawk comments found on PR after full review with dry_run=False" - ) - - -# --------------------------------------------------------------------------- -# Phase 2: Fix verification (posts results to PR) -# --------------------------------------------------------------------------- - -@integration -@needs_ado -class TestE2EFixVerification: - """Phase 2 — re-run pipeline; auto-detects prior comments, runs verify-only, posts results.""" - - @pytest.fixture(autouse=True, scope="class") - def pipeline_result(self, request): - log_path, log_handler = _setup_file_logging("fix-verify") - - settings = _setup_env() - workspace, source_branch = _clone_workspace() - _log.info("Phase 2 — Fix verification on branch: %s", source_branch) - - prior_count = _count_codehawk_comments(settings, E2E_PR_ID, E2E_REPO) - if prior_count == 0: - pytest.skip( - f"PR {E2E_PR_ID} has no existing CodeHawk comments — " - "run TestE2EFullReview first to create them." - ) - _log.info("PR has %d prior CodeHawk comments — should auto-detect re-push", prior_count) - - model = os.environ.get("OPENAI_MODEL", "gpt-4o-mini") - - job = BatchReviewJob( - pr_id=E2E_PR_ID, - repo=E2E_REPO, - workspace=workspace, - model=model, - prompt_path=REVIEW_PROMPT, - settings=settings, - review_mode=ReviewMode.FULL, - ) - - try: - phase2_error = None - phase2_output = None - findings_path = workspace / ".cr" / "findings.json" - - try: - phase2_output = job.run(dry_run=False) - except SystemExit as exc: - _log.warning("Quality gate exit code %s", exc.code) - phase2_error = exc - except Exception as exc: - _log.error("Pipeline error: %s", exc) - phase2_error = exc - - if findings_path.exists(): - findings_data = json.loads(findings_path.read_text(encoding="utf-8")) - else: - findings_data = {"findings": [], "fix_verifications": []} - - save_findings_artifact(findings_data, "e2e-fix-verify") - - _log.info("fix_verifications count: %d", len(findings_data.get("fix_verifications", []))) - for fv in findings_data.get("fix_verifications", []): - _log.info(" %s → %s: %s", fv.get("cr_id"), fv.get("status"), fv.get("reason", "")[:100]) - - if phase2_output: - log_phase2_summary(phase2_output) - - _save_run_log("fix-verify", { - "run_date": datetime.now().isoformat(), - "pr_id": E2E_PR_ID, - "repo": E2E_REPO, - "mode": "fix-verify (auto-detect, dry_run=False)", - "prior_codehawk_comments": prior_count, - "findings": findings_data, - "phase2": phase2_output, - "error": str(phase2_error) if phase2_error else None, - }) - - request.cls.findings_data = findings_data - request.cls.phase2_output = phase2_output - request.cls.phase2_error = phase2_error - request.cls.prior_count = prior_count - request.cls.workspace = workspace - finally: - _teardown_file_logging(log_handler) - _log.info("Phase 2 log saved: %s", log_path) - - # ------------------------------------------------------------------ - # Phase 2 assertions - # ------------------------------------------------------------------ - - def test_pipeline_ran(self): - assert self.findings_data is not None, "Pipeline did not produce findings" - - def test_no_new_findings(self): - findings = self.findings_data.get("findings", []) - assert findings == [], ( - f"Verify-only should produce 0 new findings, got {len(findings)}" - ) - - def test_verify_fixes_mode_set(self): - modes = self.findings_data.get("review_modes", []) - assert "verify_fixes" in modes, f"Expected 'verify_fixes' in review_modes, got {modes}" - - def test_fix_verifications_present(self): - fv_list = self.findings_data.get("fix_verifications", []) - assert len(fv_list) > 0, ( - f"Expected fix_verifications to be non-empty — pipeline should have " - f"auto-detected {self.prior_count} prior CodeHawk comments" - ) - - def test_fix_verifications_cover_prior_findings(self): - fv_list = self.findings_data.get("fix_verifications", []) - assert len(fv_list) >= self.prior_count * 0.5, ( - f"Only {len(fv_list)} fix_verifications for {self.prior_count} prior comments — " - f"expected at least 50% coverage" - ) - - def test_fix_verification_statuses_valid(self): - valid_statuses = {"fixed", "still_present", "not_relevant"} - for fv in self.findings_data.get("fix_verifications", []): - assert fv.get("status") in valid_statuses, ( - f"Invalid status '{fv.get('status')}' for {fv.get('cr_id')}" - ) - - def test_fix_verifications_have_reasons(self): - for fv in self.findings_data.get("fix_verifications", []): - reason = fv.get("reason", "") - assert reason and len(reason) > 5, ( - f"Missing or too-short reason for {fv.get('cr_id')}: '{reason}'" - ) - - def test_phase2_output_includes_fix_verifications(self): - if self.phase2_output is None: - pytest.skip("Phase 2 failed — cannot check output structure") - fv_out = self.phase2_output.get("fix_verifications", []) - assert len(fv_out) > 0, "Phase 2 output should include fix_verifications" - - def test_phase2_has_comparison_flag(self): - if self.phase2_output is None: - pytest.skip("Phase 2 failed — cannot check output structure") - assert "has_comparison" in self.phase2_output, ( - "Phase 2 output missing 'has_comparison' key" - ) - - def test_verify_comments_posted_to_pr(self): - """After fix verification with dry_run=False, PR should still have CodeHawk threads.""" - settings = _setup_env() - count = _count_codehawk_comments(settings, E2E_PR_ID, E2E_REPO) - _log.info("Post-verify cr-id comment count: %d", count) - assert count > 0, "Expected CodeHawk comments on PR after fix verification" diff --git a/tests/integration/test_fix_verification.py b/tests/integration/test_fix_verification.py deleted file mode 100644 index d06a36f..0000000 --- a/tests/integration/test_fix_verification.py +++ /dev/null @@ -1,341 +0,0 @@ -""" -Fix verification integration test — re-push flow (auto-detect). - -Runs the pipeline in default FULL mode (no flags) against a PR that already -has CodeHawk comments. The pipeline should auto-detect the re-push, fetch -prior findings, force single-pass, and produce fix_verifications[]. - -This mirrors how CI works in production — no --verify-fixes flag needed. - -Target: PR 6658 in BluSKY repo (blub0x / BluSKY Git) - -Prerequisites: - - The target PR must have existing CodeHawk comments (with markers) - - ADO_PAT and OPENAI_API_KEY must be set (via .env or environment) - -Run: - pytest tests/integration/test_fix_verification.py -v -m integration -s -""" - -import json -import logging -import os -import subprocess -from datetime import datetime -from pathlib import Path - -import pytest -from dotenv import load_dotenv - -from batch_review_job import BatchReviewJob -from config import Settings, reset_settings -from models.review_models import ReviewMode - -from .conftest import ( - PROJECT_ROOT, REVIEW_PROMPT, MAX_TURNS_INTEGRATION, - integration, needs_ado, - save_findings_artifact, log_phase2_summary, -) - -load_dotenv(PROJECT_ROOT / ".env") - -_log = logging.getLogger(__name__) - -# --------------------------------------------------------------------------- -# Test-specific constants — PR 6658 in BluSKY -# --------------------------------------------------------------------------- - -FV_PR_ID = 6658 -FV_REPO = "BluSKY" -FV_ADO_ORG = "blub0x" -FV_ADO_PROJECT = "BluSKY Git" - -RESULTS_DIR = PROJECT_ROOT / "results" - - -# --------------------------------------------------------------------------- -# Helpers -# --------------------------------------------------------------------------- - -def _setup_fv_env() -> Settings: - """Configure env vars for BluSKY repo and return fresh Settings.""" - env = { - "VCS": "ado", - "AZURE_DEVOPS_ORG": FV_ADO_ORG, - "AZURE_DEVOPS_PROJECT": FV_ADO_PROJECT, - "AZURE_DEVOPS_PAT": os.environ.get("ADO_PAT", ""), - "AZURE_DEVOPS_REPO": FV_REPO, - "AUTH_MODE": "pat", - "LOG_LEVEL": "DEBUG", - "LOG_FORMAT": "text", - "ENABLE_GRAPH": "true", - } - for k, v in env.items(): - os.environ[k] = v - reset_settings() - return Settings() - - -def _clone_fv_workspace() -> tuple[Path, str]: - """Clone the BluSKY repo and checkout the PR 6658 source branch.""" - settings = _setup_fv_env() - - from activities.fetch_pr_details_activity import FetchPRDetailsActivity - from models.review_models import FetchPRDetailsInput - - activity = FetchPRDetailsActivity(settings=settings) - pr = activity.execute(FetchPRDetailsInput(pr_id=FV_PR_ID, repository_id=FV_REPO)) - source_branch = pr.source_branch - - workspace = RESULTS_DIR / "workspace-blusky" - pat = os.environ["AZURE_DEVOPS_PAT"] - project_encoded = FV_ADO_PROJECT.replace(" ", "%20") - repo_encoded = FV_REPO.replace(" ", "%20") - auth_url = f"https://{pat}@dev.azure.com/{FV_ADO_ORG}/{project_encoded}/_git/{repo_encoded}" - - if (workspace / ".git").exists(): - _log.info("Re-using existing clone at %s", workspace) - subprocess.run( - ["git", "remote", "set-url", "origin", auth_url], - cwd=str(workspace), check=True, capture_output=True, text=True, timeout=10, - ) - subprocess.run( - ["git", "config", "remote.origin.fetch", "+refs/heads/*:refs/remotes/origin/*"], - cwd=str(workspace), check=True, capture_output=True, text=True, timeout=10, - ) - subprocess.run( - ["git", "fetch", "origin", source_branch, "--depth", "1"], - cwd=str(workspace), check=True, capture_output=True, text=True, timeout=120, - ) - subprocess.run( - ["git", "checkout", "-B", source_branch, f"origin/{source_branch}"], - cwd=str(workspace), check=True, capture_output=True, text=True, timeout=30, - ) - else: - _log.info("Cloning %s (branch: %s)...", FV_REPO, source_branch) - workspace.parent.mkdir(parents=True, exist_ok=True) - subprocess.run( - ["git", "clone", "--branch", source_branch, "--depth", "1", - auth_url, str(workspace)], - check=True, capture_output=True, text=True, timeout=600, - ) - - (workspace / ".cr").mkdir(exist_ok=True) - return workspace, source_branch - - -def _setup_file_logging() -> tuple[Path, logging.FileHandler]: - RESULTS_DIR.mkdir(exist_ok=True) - timestamp = datetime.now().strftime("%Y-%m-%d_%H%M%S") - log_path = RESULTS_DIR / f"fix-verify-{FV_PR_ID}-{timestamp}.log" - - handler = logging.FileHandler(str(log_path), mode="w", encoding="utf-8") - handler.setLevel(logging.DEBUG) - handler.setFormatter(logging.Formatter( - "%(asctime)s [%(levelname)s] %(name)s — %(message)s", - datefmt="%Y-%m-%d %H:%M:%S", - )) - - root = logging.getLogger("codehawk") - root.addHandler(handler) - root.setLevel(logging.DEBUG) - - console = logging.StreamHandler() - console.setLevel(logging.INFO) - console.setFormatter(logging.Formatter( - "%(asctime)s [%(levelname)s] %(name)s — %(message)s", - datefmt="%H:%M:%S", - )) - if not any(isinstance(h, logging.StreamHandler) and not isinstance(h, logging.FileHandler) - for h in root.handlers): - root.addHandler(console) - - _log.info("Logging to: %s", log_path) - return log_path, handler - - -def _teardown_file_logging(handler: logging.FileHandler): - logging.getLogger("codehawk").removeHandler(handler) - handler.close() - - -def _count_codehawk_comments(settings, pr_id, repo) -> int: - """Quick check: how many CodeHawk comment threads exist on this PR?""" - from activities.fetch_pr_comments_activity import FetchPRCommentsActivity - - activity = FetchPRCommentsActivity(settings=settings) - all_threads = activity.execute(pr_id=pr_id, repository_id=repo) - codehawk_threads = [t for t in all_threads if t.cr_id] - _log.info( - "Pre-check: %d total threads, %d with cr-id markers", - len(all_threads), len(codehawk_threads), - ) - for t in codehawk_threads: - _log.info( - " %s: %s (line %d in %s)", - t.cr_id, t.comment_text[:80].replace("\n", " "), - t.line_number, t.file_path, - ) - return len(codehawk_threads) - - -# --------------------------------------------------------------------------- -# Test class -# --------------------------------------------------------------------------- - -@integration -@needs_ado -class TestFixVerification: - """Run pipeline in default FULL mode — auto-detects re-push from existing PR comments.""" - - @pytest.fixture(autouse=True, scope="class") - def pipeline_result(self, request): - log_path, log_handler = _setup_file_logging() - - settings = _setup_fv_env() - workspace, source_branch = _clone_fv_workspace() - _log.info("Branch: %s", source_branch) - - # Pre-check: PR must have existing CodeHawk comments for this test to be meaningful - prior_count = _count_codehawk_comments(settings, FV_PR_ID, FV_REPO) - if prior_count == 0: - pytest.skip( - f"PR {FV_PR_ID} has no existing CodeHawk comments (no cr-id markers) — " - "cannot test fix verification. Run a normal review first to create comments." - ) - _log.info("PR has %d prior CodeHawk comments — pipeline should auto-detect re-push", prior_count) - - model = os.environ.get("OPENAI_MODEL", "gpt-4o-mini") - - # Use BatchReviewJob with default FULL mode — no --verify-fixes flag. - # The pipeline should auto-detect prior findings and handle them. - job = BatchReviewJob( - pr_id=FV_PR_ID, - repo=FV_REPO, - workspace=workspace, - model=model, - prompt_path=REVIEW_PROMPT, - settings=settings, - review_mode=ReviewMode.FULL, - ) - - try: - phase2_error = None - phase2_output = None - findings_path = workspace / ".cr" / "findings.json" - - try: - phase2_output = job.run(dry_run=True) - except SystemExit as exc: - _log.warning("Quality gate exit code %s", exc.code) - phase2_error = exc - except Exception as exc: - _log.error("Pipeline error: %s", exc) - phase2_error = exc - - if findings_path.exists(): - findings_data = json.loads(findings_path.read_text(encoding="utf-8")) - else: - findings_data = {"findings": [], "fix_verifications": []} - - save_findings_artifact(findings_data, "fix-verify") - - _log.info("fix_verifications count: %d", len(findings_data.get("fix_verifications", []))) - for fv in findings_data.get("fix_verifications", []): - _log.info(" %s → %s: %s", fv.get("cr_id"), fv.get("status"), fv.get("reason", "")[:100]) - - if phase2_output: - log_phase2_summary(phase2_output) - - # Save run log - RESULTS_DIR.mkdir(exist_ok=True) - timestamp = datetime.now().strftime("%Y-%m-%d_%H%M%S") - run_log = { - "run_date": datetime.now().isoformat(), - "pr_id": FV_PR_ID, - "repo": FV_REPO, - "mode": "verify-only (auto-detect re-push)", - "prior_codehawk_comments": prior_count, - "findings": findings_data, - "phase2": phase2_output, - "error": str(phase2_error) if phase2_error else None, - } - log_file = RESULTS_DIR / f"fix-verify-{FV_PR_ID}-{timestamp}.json" - log_file.write_text(json.dumps(run_log, indent=2), encoding="utf-8") - _log.info("Run log saved: %s", log_file) - - request.cls.findings_data = findings_data - request.cls.phase2_output = phase2_output - request.cls.phase2_error = phase2_error - request.cls.prior_count = prior_count - request.cls.workspace = workspace - finally: - _teardown_file_logging(log_handler) - _log.info("Full log saved: %s", log_path) - - # ------------------------------------------------------------------ - # Assertions - # ------------------------------------------------------------------ - - def test_pipeline_ran(self): - """Pipeline must produce findings.json.""" - assert self.findings_data is not None, "Pipeline did not produce findings" - - def test_no_new_findings(self): - """Re-push verify-only mode should skip full review — no new findings.""" - findings = self.findings_data.get("findings", []) - assert findings == [], ( - f"Verify-only should produce 0 new findings, got {len(findings)}" - ) - - def test_verify_fixes_mode_set(self): - """findings.json should have verify_fixes in review_modes.""" - modes = self.findings_data.get("review_modes", []) - assert "verify_fixes" in modes, f"Expected 'verify_fixes' in review_modes, got {modes}" - - def test_fix_verifications_present(self): - """Auto-detected re-push must produce fix_verifications.""" - fv_list = self.findings_data.get("fix_verifications", []) - assert len(fv_list) > 0, ( - f"Expected fix_verifications to be non-empty — pipeline should have " - f"auto-detected {self.prior_count} prior CodeHawk comments" - ) - - def test_fix_verifications_cover_prior_findings(self): - """fix_verifications should cover a reasonable portion of prior cr-ids.""" - fv_list = self.findings_data.get("fix_verifications", []) - assert len(fv_list) >= self.prior_count * 0.5, ( - f"Only {len(fv_list)} fix_verifications for {self.prior_count} prior comments — " - f"expected at least 50% coverage" - ) - - def test_fix_verification_statuses_valid(self): - """Each fix_verification must have a valid status.""" - valid_statuses = {"fixed", "still_present", "not_relevant"} - for fv in self.findings_data.get("fix_verifications", []): - assert fv.get("status") in valid_statuses, ( - f"Invalid status '{fv.get('status')}' for {fv.get('cr_id')}" - ) - - def test_fix_verifications_have_reasons(self): - """Each fix_verification should include a reason.""" - for fv in self.findings_data.get("fix_verifications", []): - reason = fv.get("reason", "") - assert reason and len(reason) > 5, ( - f"Missing or too-short reason for {fv.get('cr_id')}: '{reason}'" - ) - - def test_phase2_output_includes_fix_verifications(self): - """Phase 2 structured output should include fix_verifications.""" - if self.phase2_output is None: - pytest.skip("Phase 2 failed — cannot check output structure") - fv_out = self.phase2_output.get("fix_verifications", []) - assert len(fv_out) > 0, "Phase 2 output should include fix_verifications" - - def test_phase2_has_comparison_flag(self): - """Phase 2 output should set has_comparison when fix_verifications are present.""" - if self.phase2_output is None: - pytest.skip("Phase 2 failed — cannot check output structure") - assert "has_comparison" in self.phase2_output, ( - "Phase 2 output missing 'has_comparison' key" - ) diff --git a/tests/unit/test_phase2_scoring.py b/tests/unit/test_phase2_scoring.py index 763b1f5..9dc2b56 100644 --- a/tests/unit/test_phase2_scoring.py +++ b/tests/unit/test_phase2_scoring.py @@ -15,8 +15,8 @@ import post_findings as pf -PR_ID = 6571 -REPO = "BluSKYFunctionApps" +PR_ID = 1 +REPO = "TestRepo" def _write_findings(tmp_path, findings, fix_verifications=None, review_modes=None): From f184e7197311667dc6e3dcdb38c2f21ba3eccc06 Mon Sep 17 00:00:00 2001 From: Siddharth Date: Sun, 24 May 2026 12:13:35 +0530 Subject: [PATCH 4/4] fix(entrypoint): forward CLI args so --verify-fixes and --check-new-findings reach run_agent.py --- entrypoint.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/entrypoint.sh b/entrypoint.sh index 63db5b7..ec26121 100644 --- a/entrypoint.sh +++ b/entrypoint.sh @@ -39,6 +39,7 @@ python3 /app/src/run_agent.py \ --model "$OPENAI_MODEL" \ --prompt-file /app/commands/review-pr-core.md \ --commit-id "${COMMIT_ID:-}" \ - ${DRY_RUN_FLAG:-} + ${DRY_RUN_FLAG:-} \ + "$@" echo "==> codehawk complete."