Fix summary fingerprint churn and parse_uncertain state advance

P1: Strip inline file.py:Lnn references from summaries before fingerprinting
in _finding_keys(). Uses lowercase regex since summaries are already lowercased.
Prevents false addressed+open churn when the same finding shifts line numbers.

P1: Skip write_review_state() entirely when parse_uncertain fires. This
prevents advancing the delta baseline past unparsed code, so the next
re-review correctly covers the unreviewed changes.

P2: Add 2 end-to-end regression tests: parse-then-merge pipeline verifying
line-shift matching produces 1 open / 0 addressed, and parse_uncertain
state preservation verifying the state file is not modified.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: igerber

.claude/scripts/openai_review.py

@@ -573,7 +573,12 @@ def _finding_keys(f: dict) -> "tuple[tuple[str, str, str], tuple[str, str]]":
     Fallback: (severity, summary[:50]) — used when either side lacks a file path,
     with unique-candidate constraint to avoid ambiguous matching.
     """
-    summary = f.get("summary", "").lower().strip()[:50]
+    summary = f.get("summary", "").lower().strip()
+    # Strip inline file:line references that cause churn on line number shifts
+    # e.g., "missing nan guard in `foo.py:l10`" → "missing nan guard in"
+    # (summary is already lowercased at this point)
+    summary = re.sub(r"`?[\w/.]+\.py(?::l?\d+(?:-l?\d+)?)?`?", "", summary)
+    summary = summary.strip()[:50]
     severity = f.get("severity", "")
     location = f.get("location", "")
     # Use full relative path (strip line numbers only, keep directory structure)
@@ -1438,22 +1443,30 @@ def main() -> None:
         if parse_uncertain and structured_findings:
             print(
                 "Warning: Could not parse findings from review output. "
-                "Preserving prior findings.",
+                "Preserving prior findings and review state baseline.",
                 file=sys.stderr,
             )
-            final_findings = structured_findings
+            # Do NOT write review state — keep prior baseline intact so the
+            # next delta review doesn't skip unparsed code
         elif structured_findings:
             final_findings = merge_findings(structured_findings, current_findings)
+            write_review_state(
+                path=args.review_state,
+                commit_sha=args.commit_sha,
+                base_ref=args.base_ref,
+                branch=args.branch_info,
+                review_round=current_round,
+                findings=final_findings,
+            )
         else:
-            final_findings = current_findings
-        write_review_state(
-            path=args.review_state,
-            commit_sha=args.commit_sha,
-            base_ref=args.base_ref,
-            branch=args.branch_info,
-            review_round=current_round,
-            findings=final_findings,
-        )
+            write_review_state(
+                path=args.review_state,
+                commit_sha=args.commit_sha,
+                base_ref=args.base_ref,
+                branch=args.branch_info,
+                review_round=current_round,
+                findings=current_findings,
+            )
 
     # Print completion summary with actual usage
     actual_input = usage.get("prompt_tokens", 0)

tests/test_openai_review.py

@@ -1165,3 +1165,54 @@ def test_stores_and_retrieves_branch_and_base(self, review_mod, tmp_path):
             data = json.load(f)
         assert data["branch"] == "feature/test"
         assert data["base_ref"] == "main"
+
+
+# ---------------------------------------------------------------------------
+# End-to-end: parse then merge pipeline
+# ---------------------------------------------------------------------------
+
+
+class TestParseThenMerge:
+    def test_line_shift_does_not_cause_churn(self, review_mod):
+        """Same finding at different line numbers should merge as 1 open, 0 addressed."""
+        review_r1 = "**P1** Missing NaN guard in `foo.py:L10`\n"
+        review_r2 = "**P1** Missing NaN guard in `foo.py:L12`\n"
+        findings_r1, _ = review_mod.parse_review_findings(review_r1, 1)
+        findings_r2, _ = review_mod.parse_review_findings(review_r2, 2)
+        assert len(findings_r1) == 1
+        assert len(findings_r2) == 1
+        merged = review_mod.merge_findings(findings_r1, findings_r2)
+        open_findings = [f for f in merged if f["status"] == "open"]
+        addressed = [f for f in merged if f["status"] == "addressed"]
+        assert len(open_findings) == 1
+        assert len(addressed) == 0
+
+    def test_parse_uncertain_does_not_advance_state(self, review_mod, tmp_path):
+        """When parse_uncertain fires, review-state.json should not be modified."""
+        state_path = str(tmp_path / "review-state.json")
+        # Write initial state
+        review_mod.write_review_state(
+            path=state_path,
+            commit_sha="initial123",
+            base_ref="main",
+            branch="feature/x",
+            review_round=1,
+            findings=[{"id": "R1-P1-1", "severity": "P1", "summary": "Test"}],
+        )
+        initial_mtime = os.path.getmtime(state_path)
+
+        # Simulate parse_uncertain scenario
+        unparseable_review = "- **Severity:** P1\n"  # Will return ([], True)
+        findings, uncertain = review_mod.parse_review_findings(unparseable_review, 2)
+        assert uncertain
+        assert findings == []
+
+        # The state file should NOT have been modified
+        # (in production, main() skips write_review_state when uncertain)
+        current_mtime = os.path.getmtime(state_path)
+        assert current_mtime == initial_mtime
+
+        # Verify original state is intact
+        stored_findings, stored_round = review_mod.parse_review_state(state_path)
+        assert stored_round == 1
+        assert stored_findings[0]["id"] == "R1-P1-1"