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/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." 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..403c8da 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) @@ -840,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): @@ -1195,9 +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 (use mode-adjusted findings) - all_adjusted = scorer.apply_mode_multipliers(capped, findings_file.review_modes) - score = scorer.calculate_pr_score(all_adjusted) + # 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, 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 @@ -1260,8 +1248,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) @@ -1288,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 e93c001..e4339e4 100644 --- a/src/pr_scorer.py +++ b/src/pr_scorer.py @@ -259,25 +259,76 @@ 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, 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. """ - 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). + modes = {m.lower() for m in (review_modes or [])} - 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)) + 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 + + 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/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/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 # --------------------------------------------------------------------------- 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):