From d5eec2ea98f593e290936f3982ce5e1e5cbfea14 Mon Sep 17 00:00:00 2001 From: piekstra Date: Wed, 19 Aug 2026 18:12:36 -0400 Subject: [PATCH] fix(pipeline): exempt deleted files from reviewer coverage Extends the coverage-exemption introduced for generated lockfiles (#567) to deleted files. A PR that deletes files could never be approved: the deleted paths are assigned to a reviewer, which can't inspect a file that no longer exists at head, so it reports them skipped -> incomplete_skipped -> APPROVE is coerced to COMMENT. Same failure mode as a churned lockfile. Deleted paths (FilePatch.Deleted) are now excluded from the coverage universe alongside lockfiles, at both the changed-files and per-agent-scope layers, so neither a skipped nor an unassigned deletion blocks approval. Skipped files are still reported for transparency; only the coverage-status decision drops them. --- internal/pipeline/pipeline.go | 50 +++++++++++++++++++++++++----- internal/pipeline/pipeline_test.go | 42 +++++++++++++++++++++++-- 2 files changed, 81 insertions(+), 11 deletions(-) diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index 2c2c386..5b07ee5 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -861,7 +861,7 @@ func executeLLMPhases(ctx context.Context, opts Options, req Request, mode execu result.Findings = findings result.ReviewerFailures = reviewerFailures result.reviewerFastDelivered = reviewerFastDelivery(prepared.fastRequested, reviewerSessions) - reviewerCoverage := buildReviewerCoverage(selection.SelectedAgents, reviewerResults, reviewerFailures, prepared.changedFiles, reviewerToolEvidenceByAgent(reviewerSessions)) + reviewerCoverage := buildReviewerCoverage(selection.SelectedAgents, reviewerResults, reviewerFailures, prepared.changedFiles, deletedPatchPaths(prepared.parsed.Patches), reviewerToolEvidenceByAgent(reviewerSessions)) result.ReviewerCoverage = reviewerCoverage result.Sessions = appendSessionsIfPresent(result.Sessions, reviewerLedgerSessions...) @@ -2486,13 +2486,46 @@ func filterReviewableFiles(files []string) []string { return out } -func buildReviewerCoverage(selected []llm.SelectedAgent, results []llm.Findings, failures []ReviewerFailure, changedFiles []string, toolEvidence ...map[string]*llm.ReviewerToolEvidence) []reviewplan.ReviewerCoverageSummary { +// deletedPatchPaths returns the set of paths removed by the diff. A deleted file +// has no content at head for a reviewer to inspect, so it is exempt from +// coverage accounting just like a generated lockfile — otherwise a skipped +// deletion marks the reviewer incomplete_skipped and blocks approval on an +// otherwise clean review. +func deletedPatchPaths(patches []FilePatch) map[string]bool { + var deleted map[string]bool + for _, patch := range patches { + if patch.Deleted { + if deleted == nil { + deleted = map[string]bool{} + } + deleted[patch.Path] = true + } + } + return deleted +} + +// excludeFiles returns values with any member of exclude removed, preserving order. +func excludeFiles(values []string, exclude map[string]bool) []string { + if len(exclude) == 0 { + return values + } + out := make([]string, 0, len(values)) + for _, value := range values { + if !exclude[value] { + out = append(out, value) + } + } + return out +} + +func buildReviewerCoverage(selected []llm.SelectedAgent, results []llm.Findings, failures []ReviewerFailure, changedFiles []string, deleted map[string]bool, toolEvidence ...map[string]*llm.ReviewerToolEvidence) []reviewplan.ReviewerCoverageSummary { if len(selected) == 0 && len(changedFiles) == 0 { return nil } - // Generated lockfiles are not a review obligation: exclude them so neither a - // reviewer that skips one nor an unassigned lockfile blocks approval. - changedFiles = filterReviewableFiles(changedFiles) + // Generated lockfiles and deleted files are not a review obligation: exclude + // them so neither a reviewer that skips one nor an unassigned one blocks + // approval. (Deleted files have no content at head to inspect.) + changedFiles = excludeFiles(filterReviewableFiles(changedFiles), deleted) resultByAgent := make(map[string]llm.Findings, len(results)) for _, result := range results { resultByAgent[result.AgentID] = result @@ -2504,9 +2537,10 @@ func buildReviewerCoverage(selected []llm.SelectedAgent, results []llm.Findings, assigned := map[string]bool{} out := make([]reviewplan.ReviewerCoverageSummary, 0, len(selected)+1) for _, agent := range selected { - // A lockfile explicitly assigned to an agent is exempt too — the scope - // is what the reviewer is held to, and lockfiles are not reviewable. - scope := filterReviewableFiles(reviewerAssignmentScope(agent, changedFiles)) + // A lockfile or deleted file explicitly assigned to an agent is exempt + // too — the scope is what the reviewer is held to, and neither is + // reviewable content at head. + scope := excludeFiles(filterReviewableFiles(reviewerAssignmentScope(agent, changedFiles)), deleted) for _, file := range scope { assigned[file] = true } diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index 52147a1..8bfd51e 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -5060,6 +5060,7 @@ func TestBuildReviewerCoverageStatuses(t *testing.T) { []llm.Findings{{AgentID: "harness:broad", InspectedFiles: []string{"main.go", "other.go"}}}, nil, []string{"main.go", "other.go"}, + nil, ) if len(broad) != 1 || broad[0].Status != reviewerCoverageCompleteBroad { t.Fatalf("broad coverage = %#v, want complete broad", broad) @@ -5076,7 +5077,7 @@ func TestBuildReviewerCoverageStatuses(t *testing.T) { } failures := []ReviewerFailure{{AgentID: "harness:failed", Error: "model failed"}} - got := buildReviewerCoverage(selected, results, failures, []string{"api.go", "db.sql", "unassigned.go", "worker.go"}) + got := buildReviewerCoverage(selected, results, failures, []string{"api.go", "db.sql", "unassigned.go", "worker.go"}, nil) byAgent := map[string]reviewplan.ReviewerCoverageSummary{} for _, entry := range got { byAgent[entry.AgentID] = entry @@ -5108,7 +5109,7 @@ func TestBuildReviewerCoverageExemptsGeneratedLockfiles(t *testing.T) { results := []llm.Findings{ {AgentID: "rust:impl", InspectedFiles: []string{"main.go"}, SkippedFiles: []string{"Cargo.lock"}}, } - got := buildReviewerCoverage(selected, results, nil, []string{"main.go", "Cargo.lock", "yarn.lock"}) + got := buildReviewerCoverage(selected, results, nil, []string{"main.go", "Cargo.lock", "yarn.lock"}, nil) if len(got) != 1 { t.Fatalf("coverage = %#v, want a single reviewer entry (no lockfile coverage rows)", got) } @@ -5128,12 +5129,42 @@ func TestBuildReviewerCoverageExemptsGeneratedLockfiles(t *testing.T) { []llm.Findings{{AgentID: "rust:impl", InspectedFiles: []string{"Cargo.lock", "main.go"}}}, nil, []string{"main.go", "Cargo.lock"}, + nil, ) if len(inspectedLock) != 1 || !reflect.DeepEqual(inspectedLock[0].InspectedFiles, []string{"main.go"}) { t.Fatalf("inspected files = %#v, want [main.go] with the lockfile dropped", inspectedLock) } } +func TestBuildReviewerCoverageExemptsDeletedFiles(t *testing.T) { + // A reviewer that skips a deleted file (no content at head to inspect) is + // complete, not incomplete_skipped, and a deleted file left unassigned is + // not incomplete_unassigned — same coverage exemption as a lockfile. + deleted := map[string]bool{"removed.go": true, "gone.go": true} + got := buildReviewerCoverage( + []llm.SelectedAgent{{AgentID: "harness:reviewer", Files: []string{"main.go", "removed.go"}}}, + []llm.Findings{{ + AgentID: "harness:reviewer", + InspectedFiles: []string{"main.go"}, + SkippedFiles: []string{"removed.go"}, + }}, + nil, + []string{"main.go", "removed.go", "gone.go"}, + deleted, + ) + if len(got) != 1 { + t.Fatalf("coverage = %#v, want a single reviewer entry (no deleted-file rows)", got) + } + if got[0].AgentID != "harness:reviewer" || got[0].Status != reviewerCoverageCompleteBroad { + t.Fatalf("coverage = %#v, want complete despite skipped deletion", got) + } + if len(got[0].SkippedFiles) != 0 { + t.Fatalf("skipped files = %#v, want none (removed.go is exempt from coverage)", got[0].SkippedFiles) + } + // gone.go (deleted, unassigned) must not surface as an incomplete_unassigned + // coverage row that would block approval. +} + func TestEnsureSelectedGlobCoverageSkipsLockfiles(t *testing.T) { // A changed lockfile that matches an agent's globs must NOT be force-assigned // into that agent's scope — it is exempt from the coverage universe. Without @@ -5163,6 +5194,7 @@ func TestBuildReviewerCoverageUsesTypedToolEvidenceInsteadOfModelConstraint(t *t }}, nil, []string{"main.go"}, + nil, map[string]*llm.ReviewerToolEvidence{ "harness:reviewer": {DiffStatus: llm.DiffToolStatusSucceeded}, }, @@ -5201,6 +5233,7 @@ func TestBuildReviewerCoverageMarksAssignedScopeMissing(t *testing.T) { []llm.Findings{{AgentID: "harness:reviewer", InspectedFiles: []string{"main.go"}}}, nil, []string{"main.go", "other.go"}, + nil, ) if len(got) != 1 { t.Fatalf("coverage entries = %#v", got) @@ -5326,6 +5359,7 @@ func TestRebaseReviewerCohortLeavesInherentlyUnmatchedFilesUnassigned(t *testing []llm.Findings{{AgentID: "repo:go", InspectedFiles: []string{"main.go"}}}, nil, []string{"main.go", ".github/workflows/ci.yml"}, + nil, ) if len(coverage) != 2 || coverage[1].AgentID != "unassigned" || coverage[1].Status != reviewerCoverageIncompleteUnassigned || !reflect.DeepEqual(coverage[1].SkippedFiles, []string{".github/workflows/ci.yml"}) { @@ -5370,7 +5404,7 @@ func TestRebaseReviewerCohortOnlyInherentlyUnmatchedFileRemainsUnassigned(t *tes if len(selection.SelectedAgents) != 0 || len(resumes) != 0 { t.Fatalf("reused cohort selection = %#v resumes = %#v, want no assigned reviewers or resumes", selection.SelectedAgents, resumes) } - coverage := buildReviewerCoverage(selection.SelectedAgents, nil, nil, changedFiles) + coverage := buildReviewerCoverage(selection.SelectedAgents, nil, nil, changedFiles, nil) wantCoverage := []reviewplan.ReviewerCoverageSummary{{ AgentID: "unassigned", Status: reviewerCoverageIncompleteUnassigned, @@ -5526,6 +5560,7 @@ func TestBuildReviewerCoverageAllowsBroadReviewerSplitAssignments(t *testing.T) }, nil, []string{"main.go", "other.go"}, + nil, ) if len(got) != 2 { t.Fatalf("coverage entries = %#v, want two reviewer entries", got) @@ -5543,6 +5578,7 @@ func TestBuildReviewerCoverageIgnoresSkippedFilesOutsideAssignmentScope(t *testi []llm.Findings{{AgentID: "harness:reviewer", InspectedFiles: []string{"main.go"}, SkippedFiles: []string{"other.go"}}}, nil, []string{"main.go", "other.go"}, + nil, ) if len(got) != 2 { t.Fatalf("coverage entries = %#v, want reviewer plus unassigned other.go", got)