Skip to content

fix(pipeline): exclude deleted files from reviewer coverage - #570

Open
piekstra wants to merge 1 commit into
mainfrom
fix/deleted-files-coverage
Open

fix(pipeline): exclude deleted files from reviewer coverage#570
piekstra wants to merge 1 commit into
mainfrom
fix/deleted-files-coverage

Conversation

@piekstra

Copy link
Copy Markdown
Contributor

Problem

A PR that deletes files can never be approved by cr — it always posts as a comment, even with zero findings.

Chain:

  1. Deleted paths are in the diff, so they're assigned to a reviewer.
  2. A reviewer can't inspect a file that no longer exists at head, so it reports the path skipped.
  3. buildReviewerCoverage marks that reviewer incomplete_skipped (pipeline.go).
  4. buildReview (reviewplan.go) coerces an APPROVE to COMMENT whenever any reviewer coverage is incomplete.
  5. Net: a clean, zero-finding review posts as a comment; on a branch requiring one approval, the PR is unmergeable.

Observed in the wild on two PRs that delete files (a legacy router removal; a Create-React-App → Vite migration): every re-review, including local --rerun, produced outcome: comment with 0 findings across all reviewers, solely because one reviewer skipped a deleted file. A sibling PR with no deletions approved normally.

Fix

Exclude deleted paths (FilePatch.Deleted) from the coverage-completeness decision in buildReviewerCoverage:

  • A skipped deletion no longer counts toward incomplete_skipped.
  • An unassigned deletion is no longer an incomplete_unassigned gap.
  • Skipped files are still reported in full for transparency; only the status decision drops deletions.

The deletion set is derived at the call site from prepared.parsed.Patches and threaded into buildReviewerCoverage.

Tests

  • TestBuildReviewerCoverageIgnoresSkippedDeletedFiles — a reviewer skipping a deleted file stays complete, and the skip is still surfaced.
  • TestBuildReviewerCoverageDeletedUnassignedFileIsNotAGap — an unassigned deletion doesn't create an unassigned coverage entry.
  • Existing buildReviewerCoverage tests updated for the new parameter.

make lint and make test pass. Verified end-to-end by building the patched binary and re-reviewing a real deletion PR: the clean review now posts approve instead of comment.

A PR that deletes files could never be approved by cr. Deleted paths are in
the diff and get assigned to a reviewer, but a reviewer cannot inspect a file
that no longer exists at head, so it reports the path skipped. That marked the
reviewer's coverage incomplete_skipped, and buildReview coerces an APPROVE to a
COMMENT whenever any reviewer coverage is incomplete. The result: a review with
zero findings posts as a comment, and on a branch requiring one approval the PR
is unmergeable.

Exclude deleted paths (FilePatch.Deleted) from the coverage-completeness
decision: a skipped or unassigned deletion is no longer a gap, since there is
nothing at head to inspect. Skipped files are still reported in full for
transparency; only the status decision drops deletions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant