fix(pipeline): exclude deleted files from reviewer coverage

[piekstra]

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.

[piekstra-dev]

Automated PR Review

Reviewed commit: d5eec2ea98f5
Profile: reviewer - Posting as: piekstra-dev

Summary

Reviewer	Findings
go:implementation-tests	1
structure:repo-health	0

go:implementation-tests (1 finding)

Minor - internal/pipeline/pipeline.go:861

entry.SkippedFiles and entry.InspectedFiles handle deleted paths asymmetrically. SkippedFiles = sortedIntersection(result.SkippedFiles, scope) uses scope, which is already deletion-filtered (line 2543), so a reviewer that skips a deleted file has that skip dropped entirely from the coverage summary. But entry.InspectedFiles = filterReviewableFiles(copySortedStrings(result.InspectedFiles)) is only filtered against generated lockfiles, not against deleted -- so a reviewer that instead claims to have inspected a deleted file (which has no content at head) will still show it in InspectedFiles. This contradicts the PR description's stated intent ("Skipped files are still reported in full for transparency; only the status decision drops deletions") and is inconsistent with the lockfile precedent, which explicitly drops an inspected lockfile from InspectedFiles too (see TestBuildReviewerCoverageExemptsGeneratedLockfiles's inspectedLock case at pipeline_test.go:5127). TestBuildReviewerCoverageExemptsDeletedFiles only exercises the skip path and asserts SkippedFiles is empty, so it doesn't catch this gap. Fix: either filter InspectedFiles against deleted for symmetry with the lockfile exemption, or intersect SkippedFiles against the pre-deletion-exclusion scope if transparent skip reporting is actually the intended behavior, and add a test for the inspected-deleted-file case.

Reviewer Coverage

• go:implementation-tests — complete (broad); skipped: none; constraints: none
• structure:repo-health — complete (broad); skipped: none; constraints: none

Inspected files (2)

• internal/pipeline/pipeline.go
• internal/pipeline/pipeline_test.go

0 PR discussion threads considered. 0 summarized; 0 resolved.

---

Completed in 2m 15s | $1.65 | claude-sonnet-5 | cr dev

Field	Value
Model	claude-sonnet-5
Reviewers	go:implementation-tests, structure:repo-health
Engine	claude_cli · claude-sonnet-5
Reviewed by	cr · piekstra-dev
Duration	2m 15s wall · 3m 26s compute
Cost	$1.65
Tokens	76 in / 17.1k out

Per-workstream usage

• orchestrator-selection — claude-sonnet-5
  • In: 6
  • Out: 1.6k
  • Cache read: 69.8k
  • Cache create: 17.0k
  • Cost: $0.15
  • Duration: 20s
• go:implementation-tests — claude-sonnet-5
  • In: 30
  • Out: 8.3k
  • Cache read: 690.7k
  • Cache create: 54.7k
  • Cost: $0.66
  • Duration: 1m 35s
• structure:repo-health — claude-sonnet-5
  • In: 34
  • Out: 6.9k
  • Cache read: 849.1k
  • Cache create: 55.6k
  • Cost: $0.69
  • Duration: 1m 20s
• orchestrator-rollup — claude-sonnet-5
  • In: 6
  • Out: 408
  • Cache read: 85.5k
  • Cache create: 20.2k
  • Cost: $0.15
  • Duration: 10s

[piekstra-dev]

File-level note: internal/pipeline/pipeline.go

entry.SkippedFiles and entry.InspectedFiles handle deleted paths asymmetrically. SkippedFiles = sortedIntersection(result.SkippedFiles, scope) uses scope, which is already deletion-filtered (line 2543), so a reviewer that skips a deleted file has that skip dropped entirely from the coverage summary. But entry.InspectedFiles = filterReviewableFiles(copySortedStrings(result.InspectedFiles)) is only filtered against generated lockfiles, not against deleted -- so a reviewer that instead claims to have inspected a deleted file (which has no content at head) will still show it in InspectedFiles. This contradicts the PR description's stated intent ("Skipped files are still reported in full for transparency; only the status decision drops deletions") and is inconsistent with the lockfile precedent, which explicitly drops an inspected lockfile from InspectedFiles too (see TestBuildReviewerCoverageExemptsGeneratedLockfiles's inspectedLock case at pipeline_test.go:5127). TestBuildReviewerCoverageExemptsDeletedFiles only exercises the skip path and asserts SkippedFiles is empty, so it doesn't catch this gap. Fix: either filter InspectedFiles against deleted for symmetry with the lockfile exemption, or intersect SkippedFiles against the pre-deletion-exclusion scope if transparent skip reporting is actually the intended behavior, and add a test for the inspected-deleted-file case.

Reply inline to this comment.