Skip to content

Commit 4c1f0ff

Browse files
igerberclaude
andcommitted
Fix skip heuristics dropping real findings, delta budget double-count
P1: Remove _should_skip_line() from block-start matching. If a line matches _BLOCK_START, it's a finding — trust the severity pattern regardless of summary content. Skip heuristics remain in the uncertainty scanner only. Prevents findings containing "Path to Approval", "Looks good", etc. in their summaries from being silently dropped. P2: Fix delta-mode token budget to not count changed_files_text (which is NOT included in delta-mode prompt, only in fresh reviews). Prevents unnecessary import-context dropping in re-reviews. P2: Add 2 regression tests for findings with skip markers in summaries. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent cbff581 commit 4c1f0ff

2 files changed

Lines changed: 22 additions & 3 deletions

File tree

.claude/scripts/openai_review.py

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -563,9 +563,10 @@ def parse_review_findings(
563563
current_section = heading
564564
continue
565565

566-
# Check for block start
566+
# Check for block start — trust the severity pattern; don't apply
567+
# skip heuristics here (they're for the uncertainty scanner only)
567568
sev_match = _BLOCK_START.search(line)
568-
if sev_match and not _should_skip_line(line):
569+
if sev_match:
569570
# Flush previous block
570571
if current_block is not None:
571572
blocks.append((current_severity, current_block_section, current_block))
@@ -1473,8 +1474,11 @@ def main() -> None:
14731474
estimate_tokens(criteria_text)
14741475
+ estimate_tokens(registry_content)
14751476
+ estimate_tokens(diff_text)
1476-
+ estimate_tokens(changed_files_text)
14771477
)
1478+
# In delta mode, changed_files_text is NOT in the prompt (replaced by delta sections);
1479+
# only count it for fresh reviews where it appears in "Changes Under Review"
1480+
if not delta_diff_text:
1481+
mandatory_est += estimate_tokens(changed_files_text)
14781482
if previous_review:
14791483
mandatory_est += estimate_tokens(previous_review)
14801484
if delta_diff_text:

tests/test_openai_review.py

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -951,6 +951,21 @@ def test_parses_plain_label_format(self, review_mod):
951951
assert len(findings) == 1
952952
assert findings[0]["severity"] == "P2"
953953

954+
def test_finding_with_skip_marker_in_summary_still_parsed(self, review_mod):
955+
"""Findings whose summaries contain skip markers like 'Path to Approval' should parse."""
956+
review_text = "**P2** The prompt omits the Path to Approval section in `foo.py:L10`\n"
957+
findings, uncertain = review_mod.parse_review_findings(review_text, 1)
958+
assert len(findings) == 1
959+
assert findings[0]["severity"] == "P2"
960+
assert not uncertain
961+
962+
def test_finding_with_looks_good_in_summary(self, review_mod):
963+
"""Finding mentioning 'Looks good' in summary should not be skipped."""
964+
review_text = "**P1** Assessment says Looks good but edge case is unhandled in `bar.py:L5`\n"
965+
findings, _ = review_mod.parse_review_findings(review_text, 1)
966+
assert len(findings) == 1
967+
assert findings[0]["severity"] == "P1"
968+
954969
def test_parses_multiline_finding_block(self, review_mod):
955970
"""Multi-line finding blocks (Severity/Impact on separate lines)."""
956971
review_text = (

0 commit comments

Comments
 (0)