diff --git a/src/activities/fetch_pr_comments_activity.py b/src/activities/fetch_pr_comments_activity.py index e6319b3..b8ccc90 100644 --- a/src/activities/fetch_pr_comments_activity.py +++ b/src/activities/fetch_pr_comments_activity.py @@ -20,6 +20,14 @@ class FetchPRCommentsActivity(BaseActivity[int, List[ExistingCommentThread]]): """Activity to fetch existing PR comment threads from Azure DevOps.""" + _CODEHAWK_PATTERNS = ( + '## 🔴', '## ⚠️', '## 💡', + '# 🤖 AI Code Review', '# 🔍 AI Code Review', '# 🔄 AI Code Re-Review', + '**Issue Fixed**', '**Still present**', + '✅ **Accepted**', '❌ **Finding stands**', + '✅ **Dismissal accepted**', + ) + def __init__(self, settings: Settings = None): super().__init__() self.settings = settings or get_settings() @@ -220,3 +228,67 @@ def _parse_comment_markdown(self, markdown: str) -> dict: self.logger.warning(f"Failed to parse comment markdown: {e}") return result + + def get_developer_replies( + self, pr_id: int, repository_id: Optional[str] = None, + ) -> dict[str, str]: + """Fetch full conversation thread for each CodeHawk thread with developer replies. + + Returns {cr_id: formatted_conversation} where the conversation includes + all comments labeled as [CodeHawk] or [Developer Name]. + Only includes threads where at least one developer reply exists. + """ + repo_id = repository_id or self.settings.azure_devops_repo + project = self.settings.azure_devops_project + + try: + threads = self.git_client.get_threads( + repository_id=repo_id, + pull_request_id=pr_id, + project=project, + ) + except Exception as e: + self.logger.warning("Failed to fetch threads for developer replies: %s", e) + return {} + + replies: dict[str, str] = {} + for thread in threads: + if not thread.comments or len(thread.comments) < 2: + continue + + cr_id = self._extract_cr_id_from_properties(thread) or self._extract_cr_id( + thread.comments[0].content or "" + ) + if not cr_id: + continue + + has_developer_reply = False + conversation_lines: list[str] = [] + + for comment in thread.comments: + content = (comment.content or "").strip() + if not content: + continue + author = getattr(comment, "author", None) + author_unique = getattr(author, "unique_name", "") or "" + author_display = getattr(author, "display_name", "Unknown") or "Unknown" + + if self._is_codehawk_comment(content, author_unique): + conversation_lines.append(f"[CodeHawk] {content}") + else: + conversation_lines.append(f"[{author_display}] {content}") + has_developer_reply = True + + if has_developer_reply: + replies[cr_id] = "\n\n".join(conversation_lines) + + self.logger.info( + "Developer replies: %d threads with replies out of %d total", + len(replies), len(threads), + ) + return replies + + def _is_codehawk_comment(self, content: str, author_unique_name: str) -> bool: + if not author_unique_name: + return True + return any(content.startswith(p) for p in self._CODEHAWK_PATTERNS) diff --git a/src/batch_review_job.py b/src/batch_review_job.py index 3331ae4..4754406 100644 --- a/src/batch_review_job.py +++ b/src/batch_review_job.py @@ -92,6 +92,7 @@ def run(self, dry_run: bool = False, commit_id: str = "") -> Dict[str, Any]: if previous_findings: return self._run_verify_only( previous_findings, dry_run=dry_run, commit_id=commit_id, + target_commit=target_commit, ) # --- Step 3: Build graph once --- @@ -198,38 +199,93 @@ def _fetch_pr_details(self): 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 = "", + target_commit: str = "", ) -> Dict[str, Any]: - """Run fix verification only (no full review). Cheap agent-based path for re-pushes. + """Run fix verification using deterministic per-file LLM calls. - 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. + Groups findings by file, runs one LLM call per modified file with + the current content + git diff. No agent loop, no sliding window — + every finding is guaranteed to be checked. """ + from fix_verifier import verify_fixes + logger.info( - "Running fix verification agent (%s, max_turns=%d) for %d prior findings", - self._VERIFY_MODEL, self._VERIFY_MAX_TURNS, len(previous_findings), + "Running deterministic fix verification (%s) for %d prior findings", + self._VERIFY_MODEL, len(previous_findings), ) - config = ReviewJobConfig( - pr_id=self.pr_id, - repo=self.repo, + developer_replies = self._fetch_developer_replies() + + verifications, usage = verify_fixes( + old_findings=previous_findings, workspace=self.workspace, + pr_id=self.pr_id, + repo=self.repo or "", + old_commit=target_commit, 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, + settings=self.settings, + dry_run=True, + developer_replies=developer_replies, ) - job = ReviewJob(config, settings=self.settings) - return job.run(dry_run=dry_run, commit_id=commit_id) + + fixed = sum(1 for v in verifications if v.status == "fixed") + dismissed = sum(1 for v in verifications if v.status == "dismissed") + still = sum(1 for v in verifications if v.status == "still_present") + na = sum(1 for v in verifications if v.status == "not_relevant") + findings_data = { + "pr_id": self.pr_id, + "repo": self.repo, + "vcs": self.vcs, + "summary": ( + f"Fix verification complete: {fixed} fixed, {dismissed} dismissed, " + f"{still} still present, {na} not relevant " + f"(out of {len(verifications)} prior findings)." + ), + "review_modes": ["verify_fixes"], + "agent": "codex", + "findings": [], + "fix_verifications": [ + {"cr_id": v.cr_id, "status": v.status, "reason": v.reason} + for v in verifications + ], + "usage": usage, + } + + findings_path = self.workspace / ".cr" / "findings.json" + findings_path.parent.mkdir(parents=True, exist_ok=True) + findings_path.write_text(json.dumps(findings_data, indent=2), encoding="utf-8") + logger.info( + "Wrote fix verification results: %d verifications", len(verifications), + ) + + import post_findings as pf + return pf.run( + findings_path=str(findings_path), + dry_run=dry_run, + workspace=str(self.workspace), + commit_id=commit_id, + ) + + def _fetch_developer_replies(self) -> dict: + """Fetch developer replies on CodeHawk threads.""" + try: + from activities.fetch_pr_comments_activity import FetchPRCommentsActivity + activity = FetchPRCommentsActivity(settings=self.settings) + replies = activity.get_developer_replies( + pr_id=self.pr_id, repository_id=self.repo or None, + ) + if replies: + logger.info("Found %d threads with developer replies", len(replies)) + return replies + except Exception as exc: + logger.warning("Failed to fetch developer replies: %s", exc) + return {} def _fetch_previous_findings(self) -> list: """Fetch existing review threads with cr_id markers.""" diff --git a/src/fix_verifier.py b/src/fix_verifier.py index 55e705f..6fabbb7 100644 --- a/src/fix_verifier.py +++ b/src/fix_verifier.py @@ -39,6 +39,8 @@ def verify_fixes( graph_store: Any = None, settings: Optional[Settings] = None, dry_run: bool = False, + model: str = "", + developer_replies: Optional[Dict[str, str]] = None, ) -> List[FixVerification]: """Verify whether prior findings have been fixed in the current HEAD. @@ -52,6 +54,7 @@ def verify_fixes( graph_store: Optional graph store for blast-radius notes. settings: App settings (uses default if None). dry_run: If True, skip posting to ADO. + model: LLM model for verification (default: gpt-4o-mini). Returns: List of FixVerification results, one per finding. @@ -62,9 +65,7 @@ def verify_fixes( settings = settings or get_settings() workspace = Path(workspace) - deleted_files, renamed_map, modified_files = _get_file_statuses( - workspace, old_commit, new_commit - ) + diff_result = _get_file_statuses(workspace, old_commit, new_commit) findings_by_file: Dict[str, List[ExistingCommentThread]] = {} for finding in old_findings: @@ -73,59 +74,108 @@ def verify_fixes( results: List[FixVerification] = [] files_needing_llm: List[Tuple[str, List[ExistingCommentThread]]] = [] + dismissal_usage: Dict[str, int] = {"input_tokens": 0, "output_tokens": 0} + + if diff_result is None: + logger.info("Git diff unavailable — sending all %d files to LLM for verification", + len(findings_by_file)) + for file_path, file_findings in findings_by_file.items(): + files_needing_llm.append((file_path, file_findings)) + else: + deleted_files, renamed_map, modified_files = diff_result + for file_path, file_findings in findings_by_file.items(): + effective_path = renamed_map.get(file_path, file_path) + + if file_path in deleted_files or effective_path in deleted_files: + for f in file_findings: + results.append(FixVerification( + cr_id=f.cr_id or f"thread-{f.thread_id}", + status="not_relevant", + reason="File was deleted or removed from the repository.", + )) + elif file_path not in modified_files and effective_path not in modified_files: + for f in file_findings: + cr_id = f.cr_id or f"thread-{f.thread_id}" + dev_reply = (developer_replies or {}).get(cr_id) + if dev_reply: + dismissal, d_usage = _evaluate_dismissal( + f, dev_reply, effective_path, workspace, model=model, + ) + dismissal_usage["input_tokens"] += d_usage.get("input_tokens", 0) + dismissal_usage["output_tokens"] += d_usage.get("output_tokens", 0) + results.append(dismissal) + else: + results.append(FixVerification( + cr_id=cr_id, + status="still_present", + reason="File was not modified since the prior review — issue is still present.", + )) + else: + files_needing_llm.append((effective_path, file_findings)) - for file_path, file_findings in findings_by_file.items(): - effective_path = renamed_map.get(file_path, file_path) - - if file_path in deleted_files or effective_path in deleted_files: - for f in file_findings: - results.append(FixVerification( - cr_id=f.cr_id or f"thread-{f.thread_id}", - status="not_relevant", - reason="File was deleted or removed from the repository.", - )) - elif file_path not in modified_files and effective_path not in modified_files: - # File not in diff — unchanged since last review - for f in file_findings: - results.append(FixVerification( - cr_id=f.cr_id or f"thread-{f.thread_id}", - status="still_present", - reason="File was not modified since the prior review — issue is still present.", - )) - else: - files_needing_llm.append((effective_path, file_findings)) - + total_usage: Dict[str, int] = {"input_tokens": 0, "output_tokens": 0} + total_usage["input_tokens"] += dismissal_usage["input_tokens"] + total_usage["output_tokens"] += dismissal_usage["output_tokens"] if files_needing_llm: - llm_results = _verify_files_with_llm(files_needing_llm, workspace, settings) + llm_results, total_usage = _verify_files_with_llm( + files_needing_llm, workspace, settings, model=model, + base_commit=old_commit, + ) results.extend(llm_results) + if developer_replies: + dismissed_count = 0 + for i, result in enumerate(results): + if result.status != "still_present": + continue + dev_reply = developer_replies.get(result.cr_id) + if not dev_reply: + continue + finding = next((f for f in old_findings if (f.cr_id or f"thread-{f.thread_id}") == result.cr_id), None) + if not finding: + continue + file_path = finding.file_path.lstrip("/") + dismissal, dismissal_usage = _evaluate_dismissal(finding, dev_reply, file_path, workspace, model=model) + total_usage["input_tokens"] += dismissal_usage.get("input_tokens", 0) + total_usage["output_tokens"] += dismissal_usage.get("output_tokens", 0) + if dismissal.status == "dismissed": + results[i] = dismissal + dismissed_count += 1 + else: + results[i] = dismissal + if dismissed_count: + logger.info("Accepted %d developer dismissals", dismissed_count) + + total_usage["total_tokens"] = total_usage["input_tokens"] + total_usage["output_tokens"] + total_usage["model"] = model or "gpt-4o-mini" + logger.info("Verification usage: %s", total_usage) + if graph_store is not None: _annotate_blast_radius(results, old_findings, graph_store) if not dry_run: _post_results_to_ado(results, old_findings, pr_id, repo, settings) - return results + return results, total_usage def _get_file_statuses( workspace: Path, old_commit: str, new_commit: str, -) -> Tuple[set, Dict[str, str], set]: +) -> Optional[Tuple[set, Dict[str, str], set]]: """Run git diff --name-status and parse into deleted/renamed/modified sets. - Returns: - deleted_files: Set of deleted file paths. - renamed_map: Dict mapping old path to new path for renames. - modified_files: Set of modified or added file paths. + Returns None if the diff cannot be computed (missing commit, shallow clone, etc.). + Otherwise returns (deleted_files, renamed_map, modified_files). """ deleted: set = set() renamed: Dict[str, str] = {} modified: set = set() if not old_commit: - return deleted, renamed, modified + logger.warning("No old_commit provided — skipping git diff, will verify all files") + return None try: result = subprocess.run( @@ -136,8 +186,8 @@ def _get_file_statuses( timeout=30, ) if result.returncode != 0: - logger.warning("git diff --name-status failed: %s", result.stderr) - return deleted, renamed, modified + logger.warning("git diff --name-status failed: %s — will verify all files", result.stderr.strip()) + return None for line in result.stdout.splitlines(): parts = line.strip().split("\t") @@ -152,7 +202,8 @@ def _get_file_statuses( elif status.startswith(("M", "A", "C")) and len(parts) >= 2: modified.add(parts[1]) except (subprocess.TimeoutExpired, FileNotFoundError, OSError) as exc: - logger.warning("Could not run git diff --name-status: %s", exc) + logger.warning("Could not run git diff --name-status: %s — will verify all files", exc) + return None return deleted, renamed, modified @@ -161,105 +212,318 @@ def _verify_files_with_llm( files_needing_llm: List[Tuple[str, List[ExistingCommentThread]]], workspace: Path, settings: Settings, -) -> List[FixVerification]: - """Verify modified files via LLM — individual calls up to cap, then batched. + model: str = "", + base_commit: str = "", +) -> Tuple[List[FixVerification], Dict[str, int]]: + """Verify modified files via LLM — one call per file, all findings for that file. - First _INDIVIDUAL_FILE_CAP files each get their own call. - Overflow files are called _BATCH_FILE_SIZE at a time (each file still one call). + Returns (verifications, total_usage). """ results: List[FixVerification] = [] - individual = files_needing_llm[:_INDIVIDUAL_FILE_CAP] - overflow = files_needing_llm[_INDIVIDUAL_FILE_CAP:] - - for file_path, file_findings in individual: - results.extend(_verify_single_file(file_path, file_findings, workspace, settings)) + total_usage: Dict[str, int] = {"input_tokens": 0, "output_tokens": 0} + for file_path, file_findings in files_needing_llm: + file_result = _verify_single_file( + file_path, file_findings, workspace, settings, + model=model, base_commit=base_commit, + ) + if isinstance(file_result, tuple): + verifications, usage = file_result + results.extend(verifications) + total_usage["input_tokens"] += usage.get("input_tokens", 0) + total_usage["output_tokens"] += usage.get("output_tokens", 0) + else: + results.extend(file_result) + return results, total_usage - for i in range(0, len(overflow), _BATCH_FILE_SIZE): - batch = overflow[i : i + _BATCH_FILE_SIZE] - for file_path, file_findings in batch: - results.extend(_verify_single_file(file_path, file_findings, workspace, settings)) - return results +_FILE_CONTENT_CAP = 50_000 def _read_file_content(file_path: str, workspace: Path) -> str: - """Read current file content from workspace. Returns empty string on failure.""" + """Read current file content from workspace. Tries direct path, then glob fallback.""" try: full_path = workspace / file_path if full_path.exists(): return full_path.read_text(encoding="utf-8", errors="replace") + + basename = Path(file_path).name + matches = list(workspace.rglob(basename)) + if len(matches) == 1: + logger.info("Path fallback: %s -> %s", file_path, matches[0].relative_to(workspace)) + return matches[0].read_text(encoding="utf-8", errors="replace") + elif len(matches) > 1: + for m in matches: + if file_path.replace("\\", "/") in str(m.relative_to(workspace)).replace("\\", "/"): + logger.info("Path fallback (partial match): %s -> %s", file_path, m.relative_to(workspace)) + return m.read_text(encoding="utf-8", errors="replace") + + logger.warning("File not found: %s (tried %s, glob found %d matches)", file_path, full_path, len(matches)) except Exception as exc: logger.warning("Could not read file %s: %s", file_path, exc) return "" +def _extract_code_snippet(file_content: str, line_number: int, context: int = 30) -> str: + """Extract lines around the finding's line number.""" + lines = file_content.splitlines() + start = max(0, line_number - context - 1) + end = min(len(lines), line_number + context) + numbered = [f"{i + start + 1:4d} | {l}" for i, l in enumerate(lines[start:end])] + return "\n".join(numbered) + + +def _evaluate_dismissal( + finding: ExistingCommentThread, + developer_reply: str, + file_path: str, + workspace: Path, + model: str = "", +) -> Tuple[FixVerification, Dict[str, int]]: + """Evaluate whether a developer's dismissal of a finding is technically valid. + + Returns (verification_result, usage). + """ + empty_usage: Dict[str, int] = {"input_tokens": 0, "output_tokens": 0} + cr_id = finding.cr_id or f"thread-{finding.thread_id}" + file_content = _read_file_content(file_path, workspace) + if not file_content: + logger.warning("Cannot evaluate dismissal for %s — file not readable", cr_id) + return FixVerification( + cr_id=cr_id, status="still_present", + reason="File content could not be read — cannot evaluate dismissal.", + ), empty_usage + + snippet = _extract_code_snippet(file_content, finding.line_number) + codereview_md = _read_codereview_md(workspace) + prompt = _build_dismissal_prompt(finding, developer_reply, file_path, snippet, codereview_md=codereview_md) + logger.info("Evaluating dismissal for %s: \"%s\"", cr_id, developer_reply[:80]) + + result, usage = _call_llm_for_verification(prompt, 1, model=model) + if not result or not isinstance(result, list) or not result[0]: + return FixVerification( + cr_id=cr_id, status="still_present", + reason="Dismissal evaluation failed — defaulting to still_present.", + ), usage + + evaluation = result[0] + accepted = evaluation.get("accepted", False) + explanation = evaluation.get("explanation", "") + suggested_rule = evaluation.get("suggested_rule") + + if accepted: + reason = f"Developer dismissal accepted: {explanation}" + if suggested_rule: + reason += f" | Suggested .codereview.md rule: {suggested_rule}" + return FixVerification(cr_id=cr_id, status="dismissed", reason=reason), usage + + return FixVerification( + cr_id=cr_id, status="still_present", + reason=f"Developer dismissal rejected: {explanation}", + ), usage + + +def _read_codereview_md(workspace: Path) -> str: + """Read .codereview.md from the workspace root if it exists.""" + for name in (".codereview.md", "codereview.md"): + path = workspace / name + if path.exists(): + try: + return path.read_text(encoding="utf-8", errors="replace")[:10_000] + except Exception: + pass + return "" + + +def _build_dismissal_prompt( + finding: ExistingCommentThread, + thread_conversation: str, + file_path: str, + code_snippet: str, + codereview_md: str = "", +) -> str: + ext = Path(file_path).suffix.lstrip(".") + lang = {"js": "javascript", "ts": "typescript", "cs": "csharp", "py": "python"}.get(ext, ext) + + project_context = "" + if codereview_md: + project_context = f""" +## Project Context (.codereview.md) +{codereview_md} +""" + + return f"""You are evaluating whether a developer's response to a code review finding is reasonable. + +## Finding Metadata +- CR ID: {finding.cr_id} +- Severity: {finding.severity or "unknown"} +- Category: {finding.category or "unknown"} +- File: `{file_path}`, Line: {finding.line_number} +{project_context} +## Code (around the flagged line) +```{lang} +{code_snippet} +``` + +## Thread Conversation +{thread_conversation} + +## Evaluation Guidelines +You should give the developer the benefit of the doubt. Code review is collaborative, +not adversarial. The developer knows their codebase better than the reviewer. + +ACCEPT if ANY of these apply: +- The developer gives a plausible technical reason (language behavior, framework version, API design) +- The developer explains it's an intentional design decision (even without deep technical proof) +- The developer references project conventions, version constraints, or business requirements +- The code context supports the developer's claim (the code works as they describe) +- The finding is about style/convention and the developer prefers their approach + +REJECT only if: +- No reason given at all (bare "Invalid", "Won't fix" with zero explanation) +- The claim is clearly factually wrong AND you can prove it from the code +- It's a security vulnerability and the developer doesn't address the actual risk + +Consider the FULL conversation — the developer may clarify in later replies. +If the developer's reasoning is at least plausible, ACCEPT it. + +If accepted, suggest a one-line rule for .codereview.md to prevent this from being flagged again. + +Respond ONLY with JSON, no other text: +{{"accepted": true, "explanation": "...", "suggested_rule": "..."}} +or +{{"accepted": false, "explanation": "...", "suggested_rule": null}}""" + + +def _get_file_diff(file_path: str, workspace: Path, base_commit: str = "") -> str: + """Get the git diff for a specific file against the base commit.""" + base = base_commit or "HEAD~1" + try: + result = subprocess.run( + ["git", "diff", base, "HEAD", "--", file_path], + cwd=str(workspace), capture_output=True, text=True, timeout=15, + ) + if result.returncode == 0 and result.stdout.strip(): + return result.stdout[:20_000] + except Exception as exc: + logger.debug("git diff failed for %s: %s", file_path, exc) + return "" + + def _build_verification_prompt( file_path: str, file_content: str, findings: List[ExistingCommentThread], + file_diff: str = "", ) -> str: """Build the LLM verification prompt for one file with all its findings.""" rows = "\n".join( - f"| {i + 1} | {f.comment_text[:120].replace(chr(10), ' ')} | {f.line_number} |" + f"| {i + 1} | `{f.cr_id}` | {f.comment_text[:200].replace(chr(10), ' ')} | {f.line_number} |" for i, f in enumerate(findings) ) - return f"""You are verifying whether previously flagged code review findings have been fixed. -## File: {file_path} + diff_section = "" + if file_diff: + diff_section = f""" +## Git diff (what changed): +```diff +{file_diff} +``` +""" + + return f"""You are verifying whether previously flagged code review findings have been fixed. +## File: `{file_path}` +{diff_section} ## Current file content: ``` -{file_content[:8000]} +{file_content[:_FILE_CONTENT_CAP]} ``` ## Previous findings to verify: -| # | Description | Original line | -|---|-------------|---------------| +| # | cr_id | Description | Original line | +|---|-------|-------------|---------------| {rows} For each finding, determine: -- `fixed` — the specific issue described is no longer present in the current code -- `still_present` — the issue still exists (may be at a different line now) +- `fixed` — the specific issue described is no longer present in the current code. + The developer may have fixed it differently than suggested (different approach, + moved code, renamed variables, etc.) — if the underlying problem is resolved, + mark it `fixed` regardless of the exact approach used. +- `still_present` — the issue still exists in the current code (may be at a different line now) + +IMPORTANT: Look at the git diff to see what actually changed. If the diff shows the +issue was addressed, mark it `fixed`. Do not require an exact match to the original +suggestion — judge whether the underlying problem is resolved. Respond ONLY as a JSON array with no other text: [{{"finding": 1, "status": "fixed", "reason": "..."}}, ...]""" -def _call_llm_for_verification(prompt: str, expected_count: int) -> List[Dict]: - """Call OpenAI chat completions to verify findings. +def _call_llm_for_verification( + prompt: str, expected_count: int, model: str = "", +) -> Tuple[List[Dict], Dict[str, int]]: + """Call OpenAI API to verify findings. - Returns list of {finding, status, reason} dicts. - Any failure or ambiguous response defaults to still_present via _map_llm_results. + Returns (results, usage) where results is a list of {finding, status, reason} + dicts and usage is {input_tokens, output_tokens}. """ + empty_usage: Dict[str, int] = {"input_tokens": 0, "output_tokens": 0} + model = model or "gpt-4o-mini" api_key = os.environ.get("OPENAI_API_KEY", "") if not api_key: logger.warning("OPENAI_API_KEY not set — defaulting all to still_present") - return [] + return [], empty_usage try: from openai import OpenAI client = OpenAI(api_key=api_key) - response = client.chat.completions.create( - model="gpt-4o-mini", - messages=[{"role": "user", "content": prompt}], - temperature=0.0, - seed=42, - ) - raw = (response.choices[0].message.content or "").strip() - # Strip markdown fences if present + + use_responses = "codex" in model + if use_responses: + response = client.responses.create( + model=model, + input=[{"type": "message", "role": "user", "content": prompt}], + ) + raw = "" + for item in response.output: + if item.type == "message": + for content in item.content: + if hasattr(content, "text"): + raw += content.text + usage = { + "input_tokens": getattr(response.usage, "input_tokens", 0), + "output_tokens": getattr(response.usage, "output_tokens", 0), + } + else: + response = client.chat.completions.create( + model=model, + messages=[{"role": "user", "content": prompt}], + temperature=0.0, + seed=42, + ) + raw = (response.choices[0].message.content or "").strip() + resp_usage = response.usage + usage = { + "input_tokens": getattr(resp_usage, "prompt_tokens", 0) if resp_usage else 0, + "output_tokens": getattr(resp_usage, "completion_tokens", 0) if resp_usage else 0, + } + if raw.startswith("```"): raw = raw.split("```")[1] if raw.startswith("json"): raw = raw[4:] + raw = raw.strip() parsed = json.loads(raw) if isinstance(parsed, list): - return parsed - for v in parsed.values(): - if isinstance(v, list): - return v - return [] + return parsed, usage + if isinstance(parsed, dict): + for v in parsed.values(): + if isinstance(v, list): + return v, usage + return [parsed], usage + return [], usage except Exception as exc: - logger.warning("LLM verification call failed: %s", exc) - return [] + logger.warning("LLM verification call failed for model %s: %s", model, exc) + return [], empty_usage def _verify_single_file( @@ -267,6 +531,8 @@ def _verify_single_file( findings: List[ExistingCommentThread], workspace: Path, settings: Settings, + model: str = "", + base_commit: str = "", ) -> List[FixVerification]: """Verify all findings in one modified file via a single LLM call.""" file_content = _read_file_content(file_path, workspace) @@ -280,9 +546,12 @@ def _verify_single_file( for f in findings ] - prompt = _build_verification_prompt(file_path, file_content, findings) - llm_results = _call_llm_for_verification(prompt, len(findings)) - return _map_llm_results_to_verifications(findings, llm_results) + file_diff = _get_file_diff(file_path, workspace, base_commit=base_commit) + logger.info("Verifying %d findings in %s (content=%d chars, diff=%d chars)", + len(findings), file_path, len(file_content), len(file_diff)) + prompt = _build_verification_prompt(file_path, file_content, findings, file_diff=file_diff) + llm_results, usage = _call_llm_for_verification(prompt, len(findings), model=model) + return _map_llm_results_to_verifications(findings, llm_results), usage def _map_llm_results_to_verifications( diff --git a/src/models/review_models.py b/src/models/review_models.py index 6625008..25f5e70 100644 --- a/src/models/review_models.py +++ b/src/models/review_models.py @@ -219,7 +219,7 @@ class Finding: class FixVerification: """Represents a fix verification result for a prior finding.""" cr_id: str # matches a Finding.id from prior review - status: str # "fixed" | "still_present" | "not_relevant" + status: str # "fixed" | "still_present" | "not_relevant" | "dismissed" 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. diff --git a/src/post_findings.py b/src/post_findings.py index bee5a3d..5b422dd 100644 --- a/src/post_findings.py +++ b/src/post_findings.py @@ -730,15 +730,18 @@ def _build_summary_markdown( fixed = sum(1 for fv in fix_verifications if fv.status == "fixed") still = sum(1 for fv in fix_verifications if fv.status == "still_present") not_relevant = sum(1 for fv in fix_verifications if fv.status == "not_relevant") - total_verified = fixed + still + not_relevant - fix_rate = (fixed / total_verified * 100) if total_verified > 0 else 0 + dismissed = sum(1 for fv in fix_verifications if fv.status == "dismissed") + total_verified = fixed + still + not_relevant + dismissed + resolved = fixed + dismissed + fix_rate = (resolved / total_verified * 100) if total_verified > 0 else 0 lines += [ "", "## 🔧 Fix Summary", f"- ✅ **Issues Fixed:** {fixed} / {total_verified}", + f"- 💬 **Dismissals Accepted:** {dismissed} / {total_verified}" if dismissed else "", f"- ❌ **Still Present:** {still} / {total_verified}" if still else "", f"- ➖ **Not Relevant:** {not_relevant}" if not_relevant else "", - f"- **Fix Rate:** {fix_rate:.0f}%", + f"- **Resolution Rate:** {fix_rate:.0f}%", "", ] lines = [l for l in lines if l is not None and l != ""] @@ -835,6 +838,8 @@ def _build_summary_markdown( for fv in fix_verifications: if fv.status == "fixed": lines.append(f"- ✅ **{fv.cr_id}** — Fixed: {fv.reason}") + elif fv.status == "dismissed": + lines.append(f"- 💬 **{fv.cr_id}** — Dismissal accepted: {fv.reason}") elif fv.status == "still_present": lines.append(f"- ❌ **{fv.cr_id}** — Still present: {fv.reason}") elif fv.status == "not_relevant": diff --git a/tests/unit/test_dismissal_evaluation.py b/tests/unit/test_dismissal_evaluation.py new file mode 100644 index 0000000..9663800 --- /dev/null +++ b/tests/unit/test_dismissal_evaluation.py @@ -0,0 +1,242 @@ +""" +Tests for developer dismissal evaluation in fix verification. +""" + +from pathlib import Path +from unittest.mock import MagicMock, patch + +import fix_verifier as fv +from models.review_models import ExistingCommentThread + + +def _make_thread( + thread_id: int, file_path: str, cr_id: str = "", + line_number: int = 10, comment_text: str = "Test finding", + severity: str = "warning", category: str = "best_practices", +) -> ExistingCommentThread: + return ExistingCommentThread( + thread_id=thread_id, + file_path=file_path, + line_number=line_number, + status=1, + comment_text=comment_text, + created_date="2026-01-01T00:00:00", + severity=severity, + category=category, + cr_id=cr_id, + ) + + +_MOCK_USAGE = {"input_tokens": 100, "output_tokens": 50} + + +class TestIsCodehawkComment: + """Test _is_codehawk_comment heuristic.""" + + def _activity(self): + with patch("activities.fetch_pr_comments_activity.FetchPRCommentsActivity.__init__", + return_value=None): + from activities.fetch_pr_comments_activity import FetchPRCommentsActivity + act = FetchPRCommentsActivity.__new__(FetchPRCommentsActivity) + return act + + def test_severity_emoji_header(self): + act = self._activity() + assert act._is_codehawk_comment("## 🔴 CRITICAL: SQL injection", "dev@company.com") + + def test_summary_header(self): + act = self._activity() + assert act._is_codehawk_comment("# 🤖 AI Code Review\nSummary...", "dev@company.com") + + def test_fix_verified(self): + act = self._activity() + assert act._is_codehawk_comment("**Issue Fixed** — import removed", "dev@company.com") + + def test_dismissal_accepted_reply(self): + act = self._activity() + assert act._is_codehawk_comment("✅ **Dismissal accepted** — valid JS", "dev@company.com") + + def test_empty_unique_name_is_codehawk(self): + act = self._activity() + assert act._is_codehawk_comment("anything at all", "") + + def test_plain_developer_text_is_not(self): + act = self._activity() + assert not act._is_codehawk_comment( + "This is intentional, the spread on undefined is valid JS", + "developer@company.com", + ) + + +class TestExtractCodeSnippet: + def test_extracts_around_line(self): + content = "\n".join(f"line {i}" for i in range(1, 101)) + snippet = fv._extract_code_snippet(content, 50, context=5) + assert " 45 |" in snippet + assert " 55 |" in snippet + assert " 44 |" not in snippet + + def test_handles_start_of_file(self): + content = "\n".join(f"line {i}" for i in range(1, 20)) + snippet = fv._extract_code_snippet(content, 1, context=5) + assert " 1 |" in snippet + + def test_handles_end_of_file(self): + content = "\n".join(f"line {i}" for i in range(1, 20)) + snippet = fv._extract_code_snippet(content, 19, context=5) + assert " 19 |" in snippet + + +class TestEvaluateDismissal: + def _patch_file(self, content="function foo() { return 42; }"): + return patch("fix_verifier._read_file_content", return_value=content) + + def test_valid_technical_dismissal_accepted(self): + thread = _make_thread(1, "src/app.js", cr_id="cr-001") + llm_response = [{"accepted": True, "explanation": "Valid JS semantics", + "suggested_rule": "Do not flag spread on undefined"}] + with self._patch_file(): + with patch("fix_verifier._call_llm_for_verification", + return_value=(llm_response, _MOCK_USAGE)): + result, _usage = fv._evaluate_dismissal( + thread, "spreading undefined is valid JS", + "src/app.js", Path("/workspace"), + ) + assert result.status == "dismissed" + assert "Valid JS semantics" in result.reason + assert "suggested_rule" not in result.reason or ".codereview.md" in result.reason + + def test_lazy_dismissal_rejected(self): + thread = _make_thread(1, "src/app.js", cr_id="cr-001") + llm_response = [{"accepted": False, "explanation": "No technical reason given", + "suggested_rule": None}] + with self._patch_file(): + with patch("fix_verifier._call_llm_for_verification", + return_value=(llm_response, _MOCK_USAGE)): + result, _usage = fv._evaluate_dismissal( + thread, "Invalid", + "src/app.js", Path("/workspace"), + ) + assert result.status == "still_present" + assert "rejected" in result.reason.lower() + + def test_file_not_found_defaults_to_still_present(self): + thread = _make_thread(1, "src/deleted.js", cr_id="cr-001") + with patch("fix_verifier._read_file_content", return_value=""): + result, _usage = fv._evaluate_dismissal( + thread, "This is fine", + "src/deleted.js", Path("/workspace"), + ) + assert result.status == "still_present" + + def test_llm_failure_defaults_to_still_present(self): + thread = _make_thread(1, "src/app.js", cr_id="cr-001") + with self._patch_file(): + with patch("fix_verifier._call_llm_for_verification", + return_value=([], _MOCK_USAGE)): + result, _usage = fv._evaluate_dismissal( + thread, "This is intentional", + "src/app.js", Path("/workspace"), + ) + assert result.status == "still_present" + + def test_suggested_rule_included_when_accepted(self): + thread = _make_thread(1, "src/app.js", cr_id="cr-001") + llm_response = [{"accepted": True, "explanation": "Valid pattern", + "suggested_rule": "Do not flag sequential uploads in useFilePreviewUpload"}] + with self._patch_file(): + with patch("fix_verifier._call_llm_for_verification", + return_value=(llm_response, _MOCK_USAGE)): + result, _usage = fv._evaluate_dismissal( + thread, "Sequential uploads are intentional", + "src/app.js", Path("/workspace"), + ) + assert result.status == "dismissed" + assert "Do not flag sequential uploads" in result.reason + + +class TestVerifyFixesWithDismissals: + """Test that verify_fixes integrates dismissal evaluation for unchanged files.""" + + def _patch_git_modified(self, *file_paths): + stdout = "\n".join(f"M\t{p}" for p in file_paths) + mock_run = MagicMock() + mock_run.return_value = MagicMock(returncode=0, stdout=stdout, stderr="") + return patch("fix_verifier.subprocess.run", mock_run) + + def test_no_reply_stays_still_present(self): + threads = [_make_thread(1, "src/utils.js", cr_id="cr-001")] + with self._patch_git_modified("src/other.js"): + results, _usage = fv.verify_fixes( + old_findings=threads, + workspace=Path("/workspace"), + pr_id=1, repo="repo", + old_commit="abc123", + dry_run=True, + developer_replies={}, + ) + assert results[0].status == "still_present" + assert "not modified" in results[0].reason + + def test_reply_triggers_evaluation(self): + threads = [_make_thread(1, "src/utils.js", cr_id="cr-001")] + verify_response = ([{"finding": 1, "status": "still_present", "reason": "Issue remains"}], _MOCK_USAGE) + dismissal_response = ([{"accepted": True, "explanation": "Valid design", + "suggested_rule": "Allow sequential uploads"}], _MOCK_USAGE) + with self._patch_git_modified("src/other.js"): + with patch("fix_verifier._read_file_content", return_value="code here"): + with patch("fix_verifier._call_llm_for_verification", + side_effect=[verify_response, dismissal_response]): + results, _usage = fv.verify_fixes( + old_findings=threads, + workspace=Path("/workspace"), + pr_id=1, repo="repo", + old_commit="abc123", + dry_run=True, + developer_replies={"cr-001": "[CodeHawk] Finding\n\n[Dev] This is intentional"}, + ) + assert results[0].status == "dismissed" + + def test_rejected_reply_stays_still_present(self): + threads = [_make_thread(1, "src/utils.js", cr_id="cr-001")] + verify_response = ([{"finding": 1, "status": "still_present", "reason": "Issue remains"}], _MOCK_USAGE) + dismissal_response = ([{"accepted": False, "explanation": "No technical reason", + "suggested_rule": None}], _MOCK_USAGE) + with self._patch_git_modified("src/other.js"): + with patch("fix_verifier._read_file_content", return_value="code here"): + with patch("fix_verifier._call_llm_for_verification", + side_effect=[verify_response, dismissal_response]): + results, _usage = fv.verify_fixes( + old_findings=threads, + workspace=Path("/workspace"), + pr_id=1, repo="repo", + old_commit="abc123", + dry_run=True, + developer_replies={"cr-001": "[CodeHawk] Finding\n\n[Dev] Invalid"}, + ) + assert results[0].status == "still_present" + assert "rejected" in results[0].reason.lower() + + +class TestDismissedScoring: + """Verify that dismissed findings get 0 penalty in the scorer.""" + + def test_dismissed_gets_zero_penalty(self): + from pr_scorer import PRScorer + from models.review_models import FixVerification + + matrix = { + "best_practices": {"critical": 5.0, "warning": 1.0, "suggestion": 0.5, "good": 0.0}, + } + scorer = PRScorer(penalty_matrix=matrix, star_thresholds=[0.0, 5.0, 15.0, 30.0, 50.0]) + verifications = [ + FixVerification(cr_id="cr-001", status="still_present", reason="", + severity="warning", category="best_practices"), + FixVerification(cr_id="cr-002", status="dismissed", reason="Valid dismissal", + severity="warning", category="best_practices"), + FixVerification(cr_id="cr-003", status="fixed", reason="Fixed", + severity="warning", category="best_practices"), + ] + score = scorer.calculate_verify_score(verifications) + # Only cr-001 (still_present) should contribute penalty + assert score.issues_by_severity["warning"] == 1 diff --git a/tests/unit/test_phase6_fix_verification.py b/tests/unit/test_phase6_fix_verification.py index 042cabb..88a86b8 100644 --- a/tests/unit/test_phase6_fix_verification.py +++ b/tests/unit/test_phase6_fix_verification.py @@ -98,20 +98,17 @@ def test_renamed_file_detected(self): assert renamed.get("old/path/File.cs") == "new/path/File.cs" assert "new/path/File.cs" in modified - def test_empty_old_commit_returns_empty_sets(self): - deleted, renamed, modified = fv._get_file_statuses(Path("/workspace"), "", "HEAD") - assert not deleted - assert not renamed - assert not modified + def test_empty_old_commit_returns_none(self): + result = fv._get_file_statuses(Path("/workspace"), "", "HEAD") + assert result is None - def test_git_failure_returns_empty_sets(self): + def test_git_failure_returns_none(self): with patch("fix_verifier.subprocess.run") as mock_run: mock_run.return_value = MagicMock(returncode=1, stdout="", stderr="fatal error") - deleted, renamed, modified = fv._get_file_statuses( + result = fv._get_file_statuses( Path("/workspace"), "abc123", "HEAD" ) - assert not deleted - assert not modified + assert result is None # --------------------------------------------------------------------------- @@ -130,7 +127,7 @@ def test_deleted_file_all_findings_not_relevant(self): _make_thread(2, "src/auth/Login.cs", cr_id="cr-002"), ] with self._patch_git("D\tsrc/auth/Login.cs"): - results = fv.verify_fixes( + results, _usage = fv.verify_fixes( old_findings=threads, workspace=Path("/workspace"), pr_id=1, @@ -149,7 +146,7 @@ def test_unchanged_file_all_findings_still_present_no_llm(self): # git diff returns only a different file — Helper.cs not in diff with self._patch_git("M\tsrc/other/File.cs"): with patch("fix_verifier._call_llm_for_verification") as mock_llm: - results = fv.verify_fixes( + results, _usage = fv.verify_fixes( old_findings=threads, workspace=Path("/workspace"), pr_id=1, @@ -183,6 +180,8 @@ def _patch_git_modified(self, *file_paths: str): mock_run.return_value = MagicMock(returncode=0, stdout=stdout, stderr="") return patch("fix_verifier.subprocess.run", mock_run) + _MOCK_USAGE = {"input_tokens": 100, "output_tokens": 50} + def test_modified_file_3_findings_one_llm_call(self): threads = [ _make_thread(1, "src/api/Controller.cs", cr_id="cr-001"), @@ -196,8 +195,9 @@ def test_modified_file_3_findings_one_llm_call(self): ] with self._patch_git_modified("src/api/Controller.cs"): with patch("fix_verifier._read_file_content", return_value="class Ctrl {}"): - with patch("fix_verifier._call_llm_for_verification", return_value=llm_response) as mock_llm: - results = fv.verify_fixes( + with patch("fix_verifier._call_llm_for_verification", + return_value=(llm_response, self._MOCK_USAGE)) as mock_llm: + results, _usage = fv.verify_fixes( old_findings=threads, workspace=Path("/workspace"), pr_id=1, @@ -218,7 +218,8 @@ def test_5_modified_files_5_llm_calls(self): with self._patch_git_modified(*file_paths): with patch("fix_verifier._read_file_content", return_value="class X {}"): with patch("fix_verifier._call_llm_for_verification", - return_value=[{"finding": 1, "status": "fixed", "reason": "ok"}]) as mock_llm: + return_value=([{"finding": 1, "status": "fixed", "reason": "ok"}], + self._MOCK_USAGE)) as mock_llm: fv.verify_fixes( old_findings=threads, workspace=Path("/workspace"), @@ -230,13 +231,14 @@ def test_5_modified_files_5_llm_calls(self): assert mock_llm.call_count == 5 def test_20_modified_files_respects_cap(self): - """First 15 files get individual calls; remaining 5 also get individual calls (overflow).""" + """All 20 files get individual calls.""" file_paths = [f"src/file{i}.cs" for i in range(20)] threads = [_make_thread(i + 1, fp, cr_id=f"cr-{i+1:03d}") for i, fp in enumerate(file_paths)] with self._patch_git_modified(*file_paths): with patch("fix_verifier._read_file_content", return_value="class X {}"): with patch("fix_verifier._call_llm_for_verification", - return_value=[{"finding": 1, "status": "fixed", "reason": "ok"}]) as mock_llm: + return_value=([{"finding": 1, "status": "fixed", "reason": "ok"}], + self._MOCK_USAGE)) as mock_llm: fv.verify_fixes( old_findings=threads, workspace=Path("/workspace"), @@ -245,15 +247,15 @@ def test_20_modified_files_respects_cap(self): old_commit="abc123", dry_run=True, ) - # 15 individual + 5 overflow (1 per file) = 20 calls total assert mock_llm.call_count == 20 def test_llm_failure_defaults_to_still_present(self): threads = [_make_thread(1, "src/api/Login.cs", cr_id="cr-001")] with self._patch_git_modified("src/api/Login.cs"): with patch("fix_verifier._read_file_content", return_value="class X {}"): - with patch("fix_verifier._call_llm_for_verification", return_value=[]): - results = fv.verify_fixes( + with patch("fix_verifier._call_llm_for_verification", + return_value=([], self._MOCK_USAGE)): + results, _usage = fv.verify_fixes( old_findings=threads, workspace=Path("/workspace"), pr_id=1, @@ -267,8 +269,9 @@ def test_llm_garbage_json_defaults_to_still_present(self): threads = [_make_thread(1, "src/api/Login.cs", cr_id="cr-001")] with self._patch_git_modified("src/api/Login.cs"): with patch("fix_verifier._read_file_content", return_value="class X {}"): - with patch("fix_verifier._call_llm_for_verification", return_value=None): - results = fv.verify_fixes( + with patch("fix_verifier._call_llm_for_verification", + return_value=(None, self._MOCK_USAGE)): + results, _usage = fv.verify_fixes( old_findings=threads, workspace=Path("/workspace"), pr_id=1, @@ -276,7 +279,6 @@ def test_llm_garbage_json_defaults_to_still_present(self): old_commit="abc123", dry_run=True, ) - # _call_llm_for_verification returning None is handled by _map_llm_results assert results[0].status == "still_present" def test_unreadable_file_defaults_to_still_present(self): @@ -284,7 +286,7 @@ def test_unreadable_file_defaults_to_still_present(self): with self._patch_git_modified("src/api/Login.cs"): with patch("fix_verifier._read_file_content", return_value=""): with patch("fix_verifier._call_llm_for_verification") as mock_llm: - results = fv.verify_fixes( + results, _usage = fv.verify_fixes( old_findings=threads, workspace=Path("/workspace"), pr_id=1,