Skip to content

pr_review.py reports a clean round over suppressed findings it cannot see #562

Description

@ptr727

scripts/pr_review.py reports suppressed=0 for a review body that carries two suppressed findings, so the digest reads as a clean round when the reviewer has in fact raised comments that no thread will ever surface. This is a false clean in the gate that decides whether a review round is answered, which is the failure family TODO.md already names for prose_lint.py: a check that finds nothing is indistinguishable from a check with nothing to find.

Reproduction

Against the live body of the review this repository's own pull request received:

import sys; sys.path.insert(0, 'scripts')
import pr_review as p
body = <the reviewer's review body>
print(len(p.DETAILS.findall(body)))                 # 1
print(p.heading_of(p.DETAILS.findall(body)[0]))     # 'Review details'
print(len(p.suppressed_blocks(body)))               # 0
print('### Suppressed comments (2)' in body)        # True

The digest then prints suppressed=0 (on_head=0 earlier=0) while the body carries ### Suppressed comments (2), and both findings were legitimate: one on a mixed-case markup name, one on branch names formatted as code in one place and bare in another. Both were accepted and fixed once the body was read by hand.

Cause

The reviewer nests the suppressed section inside the Review details <details> block rather than giving it its own <details><summary>Suppressed comments (N)</summary>, which is the shape suppressed_blocks() was written against.

suppressed_blocks() then misses it twice over. The primary path matches SUPPRESSED against heading_of(block), which reads the wrapper's <summary>, so the block's heading is Review details and does not match. The fallback path exists for exactly this case ("the day the <details> wrapper moves") but scans DETAILS.sub('', body), which deletes every <details> block including the one the heading is nested in, so the fallback scans a body the heading is no longer in.

What a fix has to hold

  • Match the heading wherever it sits, inside a <details> block or outside one, rather than only as a wrapper's summary.
  • Count the nested heading's own (N), not the wrapper's, since finding_count() currently reads heading_of(block) and would take Review details and floor to 1 where the real count is 2.
  • A regression test carrying this body shape, so the next format change is caught by the suite rather than by a maintainer reading a review body by hand.
  • Do not narrow to the current shape. Both the old wrapper form and this nested form appear in the same pull request's rounds, so the parse handles both rather than being retargeted from one to the other.

The section wording and the --summary counter are the only surfaces involved. No other caller reads suppressed_blocks().

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions