From 017312c1c94f9356aa38d7c72935f80a1436da37 Mon Sep 17 00:00:00 2001 From: Siddharth Date: Mon, 25 May 2026 21:55:42 +0530 Subject: [PATCH 1/2] fix(verify): replace agent-based fix verification with deterministic per-file LLM calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The agent-based verifier used a sliding window of 3 turns, causing it to forget previously read files and loop endlessly — re-reading the same 5-6 files across 57 turns while never reaching the remaining 20+. This made verification unreliable (findings marked "still_present" without checking) and expensive (836K tokens / $1.78). Switch to the deterministic fix_verifier which makes one LLM call per file with all findings for that file. Key improvements: - Model: configurable, defaults to gpt-5-codex (was hardcoded gpt-4o-mini) - File content cap: 50K chars (was 8K, truncating large files) - Git diff: included in prompt so LLM sees exactly what changed - Prompt: judges whether underlying problem is resolved, not exact match - Path fallback: glob-based file resolution when ADO paths don't match - Usage tracking: token counts and cost reported in findings.json - Graceful diff failure: when git diff fails (shallow clone), verifies all files via LLM instead of marking everything "not modified" Result: 77K tokens / $0.19 (10x cheaper), all 31 findings verified, 10 correctly identified as fixed vs 0 before. --- src/batch_review_job.py | 71 ++++-- src/fix_verifier.py | 255 ++++++++++++++------- tests/unit/test_phase6_fix_verification.py | 48 ++-- 3 files changed, 256 insertions(+), 118 deletions(-) diff --git a/src/batch_review_job.py b/src/batch_review_job.py index 3331ae4..2f9b54f 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,74 @@ 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, + 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, + ) + + fixed = sum(1 for v in verifications if v.status == "fixed") + 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, " + 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, ) - 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.""" diff --git a/src/fix_verifier.py b/src/fix_verifier.py index 55e705f..cb10da4 100644 --- a/src/fix_verifier.py +++ b/src/fix_verifier.py @@ -39,6 +39,7 @@ def verify_fixes( graph_store: Any = None, settings: Optional[Settings] = None, dry_run: bool = False, + model: str = "", ) -> List[FixVerification]: """Verify whether prior findings have been fixed in the current HEAD. @@ -52,6 +53,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 +64,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: @@ -74,58 +74,71 @@ def verify_fixes( results: List[FixVerification] = [] files_needing_llm: List[Tuple[str, List[ExistingCommentThread]]] = [] - 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)) + 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: + 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} 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) + 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 +149,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 +165,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 +175,185 @@ 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 _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 + return parsed, usage for v in parsed.values(): if isinstance(v, list): - return v - return [] + return v, 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 +361,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 +376,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/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, From c57fce8cbd639ad588400ae46eb77bfea98f82a5 Mon Sep 17 00:00:00 2001 From: Siddharth Date: Mon, 25 May 2026 23:06:40 +0530 Subject: [PATCH 2/2] feat(verify): add developer dismissal evaluation to fix verification When a developer replies to a CodeHawk finding with a technical reason (e.g., "spreading undefined is valid JS", "sequential uploads are intentional"), the fix verifier now evaluates the dismissal using an LLM call with the full thread conversation and code context. Key changes: - fetch_pr_comments_activity: add get_developer_replies() returning full conversation threads labeled [CodeHawk] / [Developer Name] - fix_verifier: post-processing step evaluates still_present findings that have developer replies; accepted dismissals get status="dismissed" with 0 penalty (same as fixed) - Prompt includes .codereview.md for project context (versions, conventions) - Collaborative tone: benefit of the doubt for plausible technical reasons; reject only bare "Invalid" or provably wrong claims - Token usage from dismissal LLM calls tracked in total cost - JSON parser handles single-object responses (not just arrays) - post_findings: displays dismissed findings with resolution rate --- src/activities/fetch_pr_comments_activity.py | 72 ++++++ src/batch_review_job.py | 21 +- src/fix_verifier.py | 186 +++++++++++++- src/models/review_models.py | 2 +- src/post_findings.py | 11 +- tests/unit/test_dismissal_evaluation.py | 242 +++++++++++++++++++ 6 files changed, 521 insertions(+), 13 deletions(-) create mode 100644 tests/unit/test_dismissal_evaluation.py 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 2f9b54f..4754406 100644 --- a/src/batch_review_job.py +++ b/src/batch_review_job.py @@ -220,6 +220,8 @@ def _run_verify_only( self._VERIFY_MODEL, len(previous_findings), ) + developer_replies = self._fetch_developer_replies() + verifications, usage = verify_fixes( old_findings=previous_findings, workspace=self.workspace, @@ -229,9 +231,11 @@ def _run_verify_only( model=self._VERIFY_MODEL, settings=self.settings, dry_run=True, + developer_replies=developer_replies, ) 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 = { @@ -239,7 +243,7 @@ def _run_verify_only( "repo": self.repo, "vcs": self.vcs, "summary": ( - f"Fix verification complete: {fixed} fixed, " + f"Fix verification complete: {fixed} fixed, {dismissed} dismissed, " f"{still} still present, {na} not relevant " f"(out of {len(verifications)} prior findings)." ), @@ -268,6 +272,21 @@ def _run_verify_only( 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.""" try: diff --git a/src/fix_verifier.py b/src/fix_verifier.py index cb10da4..6fabbb7 100644 --- a/src/fix_verifier.py +++ b/src/fix_verifier.py @@ -40,6 +40,7 @@ def verify_fixes( 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. @@ -73,6 +74,7 @@ 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", @@ -93,15 +95,27 @@ def verify_fixes( )) elif file_path not in modified_files and effective_path not in modified_files: 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.", - )) + 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)) 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, total_usage = _verify_files_with_llm( files_needing_llm, workspace, settings, model=model, @@ -109,6 +123,29 @@ def verify_fixes( ) 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) @@ -226,6 +263,137 @@ def _read_file_content(file_path: str, workspace: Path) -> str: 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" @@ -347,9 +515,11 @@ def _call_llm_for_verification( parsed = json.loads(raw) if isinstance(parsed, list): return parsed, usage - for v in parsed.values(): - if isinstance(v, list): - return v, 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 for model %s: %s", model, exc) 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