From f6d1fe93c6b066ae7d3950956e94b5aa5ae20151 Mon Sep 17 00:00:00 2001
From: Aaron Wong <6979793+zzwong@users.noreply.github.com>
Date: Thu, 10 Sep 2026 15:30:58 -0400
Subject: [PATCH 1/5] fix(review): preserve deleted files in reconstructed
 diffs

---
 internal/gitprovider/github/rest.go      | 19 ++++++--
 internal/gitprovider/github/rest_test.go |  5 +++
 internal/pipeline/diff_test.go           | 27 +++++++++++
 internal/pipeline/pipeline.go            | 57 ++++++++++++++++++++++--
 internal/pipeline/pipeline_test.go       | 56 +++++++++++++++++++++++
 5 files changed, 157 insertions(+), 7 deletions(-)

diff --git a/internal/gitprovider/github/rest.go b/internal/gitprovider/github/rest.go
index 760da814..ec91bac6 100644
--- a/internal/gitprovider/github/rest.go
+++ b/internal/gitprovider/github/rest.go
@@ -179,9 +179,22 @@ func reconstructUnifiedDiff(files []pullFileResponse) string {
 		}
 		if f.Patch == "" {
 			// A pure rename has no patch and is already fully described above.
-			// Anything else without a patch is a binary or oversized file; mark
-			// it so the change stays visible even though there are no hunks.
-			if f.Status != "renamed" {
+			// A removed file also has no patch when it is binary or individually
+			// oversized. Preserve its deletion metadata instead of falling through
+			// to the generic binary marker; otherwise the diff parser classifies
+			// the removal as a modified file and reviewer coverage includes the
+			// deleted path.
+			switch f.Status {
+			case "removed":
+				fmt.Fprintf(&b, "deleted file mode 100644\n")
+				fmt.Fprintf(&b, "--- a/%s\n", oldPath)
+				b.WriteString("+++ /dev/null\n")
+			case "renamed":
+				// The rename metadata above fully describes a pure rename.
+			default:
+				// Anything else without a patch is a binary or oversized file;
+				// mark it so the change stays visible even though there are no
+				// hunks.
 				fmt.Fprintf(&b, "Binary files a/%s and b/%s differ\n", oldPath, newPath)
 			}
 			continue
diff --git a/internal/gitprovider/github/rest_test.go b/internal/gitprovider/github/rest_test.go
index 7976da11..7d7be458 100644
--- a/internal/gitprovider/github/rest_test.go
+++ b/internal/gitprovider/github/rest_test.go
@@ -459,6 +459,8 @@ func TestReconstructUnifiedDiff(t *testing.T) {
 		{Filename: "mod.go", Status: "modified", Patch: "@@ -1,2 +1,2 @@\n line\n-old\n+new"},
 		{Filename: "rn.go", PreviousFilename: "ro.go", Status: "renamed"}, // pure rename: no patch
 		{Filename: "bin.dat", Status: "added"},                            // added, no patch: binary
+		{Filename: "removed.txt", Status: "removed"},                      // removed, no patch: oversized text
+		{Filename: "removed.bin", Status: "removed"},                      // removed, no patch: binary
 	})
 	cases := []struct {
 		substr string
@@ -468,6 +470,9 @@ func TestReconstructUnifiedDiff(t *testing.T) {
 		{"diff --git a/ro.go b/rn.go\nrename from ro.go\nrename to rn.go\n", true},
 		{"Binary files a/ro.go", false}, // a pure rename must not be marked binary
 		{"diff --git a/bin.dat b/bin.dat\nBinary files a/bin.dat and b/bin.dat differ\n", true},
+		{"diff --git a/removed.txt b/removed.txt\ndeleted file mode 100644\n--- a/removed.txt\n+++ /dev/null\n", true},
+		{"diff --git a/removed.bin b/removed.bin\nBinary files a/removed.bin and b/removed.bin differ\n", false},
+		{"diff --git a/removed.bin b/removed.bin\ndeleted file mode 100644\n--- a/removed.bin\n+++ /dev/null\n", true},
 		{"+y", false}, // the empty-filename entry must be dropped entirely
 	}
 	for _, c := range cases {
diff --git a/internal/pipeline/diff_test.go b/internal/pipeline/diff_test.go
index 31c59cd3..82727535 100644
--- a/internal/pipeline/diff_test.go
+++ b/internal/pipeline/diff_test.go
@@ -156,3 +156,30 @@ func TestParseUnifiedDiffRejectsBadHunkHeader(t *testing.T) {
 		t.Fatal("parseUnifiedDiff error = nil, want bad hunk failure")
 	}
 }
+
+func TestParseUnifiedDiffPreservesPatchlessRemovedStatus(t *testing.T) {
+	raw := strings.Join([]string{
+		"diff --git a/removed.txt b/removed.txt",
+		"deleted file mode 100644",
+		"--- a/removed.txt",
+		"+++ /dev/null",
+		"diff --git a/removed.bin b/removed.bin",
+		"deleted file mode 100644",
+		"--- a/removed.bin",
+		"+++ /dev/null",
+		"",
+	}, "\n")
+
+	parsed, err := parseUnifiedDiff(raw)
+	if err != nil {
+		t.Fatalf("parseUnifiedDiff: %v", err)
+	}
+	if len(parsed.Patches) != 2 {
+		t.Fatalf("patches = %#v, want two patchless deletions", parsed.Patches)
+	}
+	for _, patch := range parsed.Patches {
+		if !patch.Deleted || patch.Binary || patch.Path == "" || patch.OldPath != patch.Path || len(patch.Hunks) != 0 {
+			t.Fatalf("patchless deletion = %#v, want Deleted with no Binary flag or hunks", patch)
+		}
+	}
+}
diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go
index 5c3a2ec3..2f95ff2b 100644
--- a/internal/pipeline/pipeline.go
+++ b/internal/pipeline/pipeline.go
@@ -780,7 +780,7 @@ func executeLLMPhases(ctx context.Context, opts Options, req Request, mode execu
 	}
 
 	cohortScope := ledger.ReviewerCohortScope{PRKey: prepared.prKey, Profile: req.ProfileName, PostingIdentity: runlifecycle.PostingKey(req.PostingIdentity)}
-	selection, reviewerResumeIDs, reusedCohort, err := loadReviewerCohort(ctx, opts, req, cohortScope, prepared.catalog, prepared.changedFiles, maxAgents)
+	selection, reviewerResumeIDs, reusedCohort, err := loadReviewerCohort(ctx, opts, req, cohortScope, prepared.catalog, reviewablePatchPaths(prepared.parsed.Patches), maxAgents)
 	if err != nil {
 		return executionPhaseFailure(err)
 	}
@@ -1254,6 +1254,12 @@ func runSelectionPhase(ctx context.Context, opts Options, req selectionPhaseRequ
 	if err != nil {
 		return llm.Selection{}, sessionDraft{}, ledger.Session{}, err
 	}
+	// Deleted files remain in the dossier so the change is visible to the
+	// orchestrator, but they are not reviewer obligations: there is no file at
+	// the head for a reviewer to inspect. Keep them out of the assignment
+	// contract and the post-selection backstops, matching buildReviewerCoverage.
+	reviewerFiles := reviewablePatchPaths(req.ParsedDiff.Patches)
+	promptInput.ChangedFiles = append([]string(nil), reviewerFiles...)
 	dependencyTaskIDs := []string{dossier.SummaryTaskID}
 	fingerprintDeps := append(append([]string(nil), dependencyTaskIDs...), promptDeps...)
 	selectionPrompt, err := buildSelectionPrompt(req.Catalog, promptInput, req.MaxAgents, req.SelectionPromptInstructions)
@@ -1270,7 +1276,7 @@ func runSelectionPhase(ctx context.Context, opts Options, req selectionPhaseRequ
 	decode := func(data []byte) (llm.Selection, error) {
 		return llm.DecodeSelection(data, llm.SelectionOptions{
 			KnownAgents:  knownAgents(req.Catalog),
-			ChangedFiles: changedFiles(req.ParsedDiff.Patches),
+			ChangedFiles: stringSet(reviewerFiles),
 			KnownThreads: knownThreadIDs,
 		})
 	}
@@ -1303,7 +1309,8 @@ func runSelectionPhase(ctx context.Context, opts Options, req selectionPhaseRequ
 	if err != nil {
 		return llm.Selection{}, selectionSession, ledgerSession, err
 	}
-	changed := patchPaths(req.ParsedDiff.Patches)
+	changed := reviewerFiles
+	selection = filterSelectedReviewerAssignments(selection, changed)
 	selection = ensureRequiredOnMatchAgents(selection, req.Catalog, changed)
 	selection, err = opts.capSelectionAgents(selection, req.Catalog, changed, req.MaxAgents)
 	if err != nil {
@@ -2056,7 +2063,8 @@ func runReviewer(ctx context.Context, opts Options, req Request, runID string, p
 		return llm.Findings{}, sessionDraft{}, ledger.Session{}, nil, Failure(FailureTerminal, err)
 	}
 	model, effort := runtimeConfig.model, runtimeConfig.effort
-	changedFilePaths := patchPaths(parsed.Patches)
+	changedFilePaths := reviewablePatchPaths(parsed.Patches)
+	selected = filterSelectedReviewerAssignment(selected, changedFilePaths)
 	assignmentScope := reviewerAssignmentScope(selected, changedFilePaths)
 	prompt, promptDeps, err := buildReviewerPrompt(artifacts, pr, selected, agent, changedFilePaths, resumeState.discussion)
 	if err != nil {
@@ -2622,6 +2630,13 @@ func deletedPatchPaths(patches []FilePatch) map[string]bool {
 	return deleted
 }
 
+// reviewablePatchPaths returns changed paths that a reviewer can inspect at
+// the head. Deleted paths remain in ParsedDiff and the dossier, but are not a
+// reviewer assignment or coverage obligation.
+func reviewablePatchPaths(patches []FilePatch) []string {
+	return excludeFiles(patchPaths(patches), deletedPatchPaths(patches))
+}
+
 // excludeFiles returns values with any member of exclude removed, preserving order.
 func excludeFiles(values []string, exclude map[string]bool) []string {
 	if len(exclude) == 0 {
@@ -2636,6 +2651,40 @@ func excludeFiles(values []string, exclude map[string]bool) []string {
 	return out
 }
 
+func filterAssignmentFiles(files, changedFiles []string) []string {
+	changed := stringSet(changedFiles)
+	var filtered []string
+	for _, file := range files {
+		if changed[file] && !slices.Contains(filtered, file) {
+			filtered = append(filtered, file)
+		}
+	}
+	return copySortedStrings(filtered)
+}
+
+func filterSelectedReviewerAssignment(selected llm.SelectedAgent, changedFiles []string) llm.SelectedAgent {
+	selected.Files = filterAssignmentFiles(selected.Files, changedFiles)
+	selected.AllowedFiles = filterAssignmentFiles(selected.AllowedFiles, changedFiles)
+	return selected
+}
+
+// filterSelectedReviewerAssignments removes deleted paths from explicit
+// assignments and drops a selected reviewer whose only assignment was
+// deleted. Broad selections remain broad over changed, reviewable paths.
+func filterSelectedReviewerAssignments(selection llm.Selection, changedFiles []string) llm.Selection {
+	filtered := selection
+	filtered.SelectedAgents = nil
+	for _, selected := range selection.SelectedAgents {
+		hadExplicitAssignment := len(selected.Files) > 0 || len(selected.AllowedFiles) > 0
+		selected = filterSelectedReviewerAssignment(selected, changedFiles)
+		if hadExplicitAssignment && len(selected.Files) == 0 && len(selected.AllowedFiles) == 0 {
+			continue
+		}
+		filtered.SelectedAgents = append(filtered.SelectedAgents, selected)
+	}
+	return filtered
+}
+
 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
diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go
index 6417a11a..ac2254e5 100644
--- a/internal/pipeline/pipeline_test.go
+++ b/internal/pipeline/pipeline_test.go
@@ -5315,6 +5315,62 @@ func TestEnsureSelectedGlobCoverageSkipsLockfiles(t *testing.T) {
 	}
 }
 
+func TestReviewablePatchPathsKeepDeletedFilesOutOfAssignments(t *testing.T) {
+	patches := []FilePatch{
+		{Path: "main.go"},
+		{OldPath: "removed.go", Path: "removed.go", Deleted: true},
+	}
+	reviewable := reviewablePatchPaths(patches)
+	if !reflect.DeepEqual(reviewable, []string{"main.go"}) {
+		t.Fatalf("reviewable paths = %#v, want only retained file", reviewable)
+	}
+
+	filtered := filterSelectedReviewerAssignments(llm.Selection{SelectedAgents: []llm.SelectedAgent{
+		{AgentID: "deleted-only", Files: []string{"removed.go"}, AllowedFiles: []string{"removed.go"}},
+		{AgentID: "mixed", Files: []string{"main.go", "removed.go"}, AllowedFiles: []string{"main.go", "removed.go"}},
+		{AgentID: "broad"},
+	}}, reviewable)
+	if len(filtered.SelectedAgents) != 2 {
+		t.Fatalf("filtered selection = %#v, want deleted-only reviewer removed", filtered.SelectedAgents)
+	}
+	if !reflect.DeepEqual(filtered.SelectedAgents[0].Files, []string{"main.go"}) ||
+		!reflect.DeepEqual(filtered.SelectedAgents[0].AllowedFiles, []string{"main.go"}) {
+		t.Fatalf("mixed assignment = %#v, want only retained file", filtered.SelectedAgents[0])
+	}
+	if len(filtered.SelectedAgents[1].Files) != 0 || len(filtered.SelectedAgents[1].AllowedFiles) != 0 {
+		t.Fatalf("broad assignment = %#v, want broad reviewer unchanged", filtered.SelectedAgents[1])
+	}
+
+	catalog := agents.Catalog{Agents: []agents.Agent{{ID: "mixed", FileGlobs: []string{"**/*.go"}}}}
+	selection := ensureSelectedGlobCoverage(llm.Selection{SelectedAgents: []llm.SelectedAgent{
+		{AgentID: "mixed", Files: []string{"main.go"}},
+	}}, catalog, reviewable)
+	if !reflect.DeepEqual(selection.SelectedAgents[0].Files, []string{"main.go"}) {
+		t.Fatalf("glob assignment = %#v, want deleted path excluded", selection.SelectedAgents[0].Files)
+	}
+}
+
+func TestRebaseReviewerCohortWithReviewablePathsExcludesDeletedFiles(t *testing.T) {
+	req := Request{Profile: testProfile(""), ProfileName: "default"}
+	cohort := ledger.ReviewerCohort{Adapter: "fake-llm", Members: []ledger.ReviewerCohortMember{{
+		AgentID: "shared:general", AssignmentMode: ledger.ReviewerAssignmentBroad,
+		Model: "claude-sonnet-5", Effort: "medium",
+	}}}
+	catalog := agents.Catalog{Agents: []agents.Agent{{ID: "shared:general", ModelTier: "medium", Effort: "medium"}}}
+	changed := reviewablePatchPaths([]FilePatch{
+		{Path: "main.go"},
+		{OldPath: "removed.go", Path: "removed.go", Deleted: true},
+	})
+
+	selection, _, err := rebaseReviewerCohort(req, catalog, cohort, changed, 0, "fake-llm")
+	if err != nil {
+		t.Fatalf("rebaseReviewerCohort: %v", err)
+	}
+	if len(selection.SelectedAgents) != 1 || !reflect.DeepEqual(selection.SelectedAgents[0].Files, []string{"main.go"}) {
+		t.Fatalf("rebased selection = %#v, want only retained file", selection.SelectedAgents)
+	}
+}
+
 func TestBuildReviewerCoverageUsesTypedToolEvidenceInsteadOfModelConstraint(t *testing.T) {
 	got := buildReviewerCoverage(
 		[]llm.SelectedAgent{{AgentID: "harness:reviewer", Files: []string{"main.go"}}},

From 215dabd2219059631fda3e53f334bf9ce2934d3c Mon Sep 17 00:00:00 2001
From: Aaron Wong <6979793+zzwong@users.noreply.github.com>
Date: Thu, 10 Sep 2026 16:38:07 -0400
Subject: [PATCH 2/5] fix(review): skip reviewers for deletion-only diffs

---
 internal/pipeline/pipeline.go      |   5 +-
 internal/pipeline/pipeline_test.go | 101 +++++++++++++++++++++++++++++
 2 files changed, 105 insertions(+), 1 deletion(-)

diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go
index 2f95ff2b..19b567ed 100644
--- a/internal/pipeline/pipeline.go
+++ b/internal/pipeline/pipeline.go
@@ -2670,10 +2670,13 @@ func filterSelectedReviewerAssignment(selected llm.SelectedAgent, changedFiles [
 
 // filterSelectedReviewerAssignments removes deleted paths from explicit
 // assignments and drops a selected reviewer whose only assignment was
-// deleted. Broad selections remain broad over changed, reviewable paths.
+// deleted. Broad selections remain broad when reviewable paths exist.
 func filterSelectedReviewerAssignments(selection llm.Selection, changedFiles []string) llm.Selection {
 	filtered := selection
 	filtered.SelectedAgents = nil
+	if len(changedFiles) == 0 {
+		return filtered
+	}
 	for _, selected := range selection.SelectedAgents {
 		hadExplicitAssignment := len(selected.Files) > 0 || len(selected.AllowedFiles) > 0
 		selected = filterSelectedReviewerAssignment(selected, changedFiles)
diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go
index ac2254e5..51180b1b 100644
--- a/internal/pipeline/pipeline_test.go
+++ b/internal/pipeline/pipeline_test.go
@@ -5350,6 +5350,92 @@ func TestReviewablePatchPathsKeepDeletedFilesOutOfAssignments(t *testing.T) {
 	}
 }
 
+func TestFilterSelectedReviewerAssignmentsDropsAllReviewersWhenNoFilesAreReviewable(t *testing.T) {
+	threadActions := []review.ThreadAction{{
+		ThreadID: "thread-1",
+		Decision: review.ThreadDecisionSummarizeOnly,
+		Summary:  "Keep the existing thread action.",
+	}}
+	selection := llm.Selection{
+		SelectedAgents: []llm.SelectedAgent{
+			{AgentID: "explicit", Files: []string{"removed.go"}, AllowedFiles: []string{"removed.go"}},
+			{AgentID: "broad"},
+		},
+		ThreadActions: threadActions,
+		Reasoning:     "the diff only deletes files",
+	}
+
+	filtered := filterSelectedReviewerAssignments(selection, nil)
+	if len(filtered.SelectedAgents) != 0 {
+		t.Fatalf("selected agents = %#v, want none without reviewable files", filtered.SelectedAgents)
+	}
+	if !reflect.DeepEqual(filtered.ThreadActions, threadActions) {
+		t.Fatalf("thread actions = %#v, want %#v", filtered.ThreadActions, threadActions)
+	}
+	if filtered.Reasoning != selection.Reasoning {
+		t.Fatalf("reasoning = %q, want %q", filtered.Reasoning, selection.Reasoning)
+	}
+}
+
+func TestDryRunDeletionOnlyDiffDoesNotRunSelectedReviewer(t *testing.T) {
+	ctx := context.Background()
+	store := openPipelineStore(t)
+	defer closeStore(t, store)
+	provider, req := dryRunHarness(t)
+
+	gitCommandMustSucceed(t, provider.fixtureRepoDir, "rm", "main.go")
+	gitCommandMustSucceed(t, provider.fixtureRepoDir, "commit", "-m", "delete main")
+	provider.pr.Head.SHA = gitCommandMustSucceed(t, provider.fixtureRepoDir, "rev-parse", "HEAD")
+	provider.diff.Raw = deletionDiff("main.go")
+
+	adapter := &llm.FakeAdapter{NameValue: "fake-llm"}
+	adapter.Queue(fakeLLMResult("selection-session", `{
+		"schema_version": 1,
+		"selected_agents": [{
+			"agent_id": "harness:reviewer",
+			"rationale": "review the whole change",
+			"files": []
+		}],
+		"thread_actions": [],
+		"reasoning": "the diff only deletes files"
+	}`, 10, 2))
+	adapter.Queue(fakeLLMResult("rollup-session", rollupJSON("approve", nil), 10, 2))
+
+	result, err := dryRunForTest(ctx, Options{
+		Provider:        provider,
+		Adapter:         adapter,
+		Store:           store,
+		Layout:          statepaths.NewLayout(t.TempDir(), t.TempDir()),
+		Now:             fixedNow,
+		NewRunID:        func() string { return "run-deletion-only" },
+		NewSessionRowID: sequence("session"),
+		NewFindingID:    findingSequence("finding"),
+		NewActionID:     actionSequence(),
+		MaxConcurrency:  1,
+	}, req)
+	if err != nil {
+		t.Fatalf("DryRun: %v", err)
+	}
+	if len(result.Selection.SelectedAgents) != 0 {
+		t.Fatalf("selected agents = %#v, want none for deletion-only diff", result.Selection.SelectedAgents)
+	}
+	if result.Selection.Reasoning != "the diff only deletes files" {
+		t.Fatalf("selection reasoning = %q, want preserved reasoning", result.Selection.Reasoning)
+	}
+	if len(result.ReviewerFailures) != 0 || len(result.ReviewerCoverage) != 0 {
+		t.Fatalf("reviewer state = failures %#v coverage %#v, want no reviewer work", result.ReviewerFailures, result.ReviewerCoverage)
+	}
+	requests := adapter.Requests()
+	if len(requests) != 2 {
+		t.Fatalf("adapter requests = %d, want selection/rollup only", len(requests))
+	}
+	for _, request := range requests {
+		if strings.Contains(request.Prompt, `"schema": "findings"`) {
+			t.Fatalf("unexpected reviewer request for deletion-only diff:\n%s", request.Prompt)
+		}
+	}
+}
+
 func TestRebaseReviewerCohortWithReviewablePathsExcludesDeletedFiles(t *testing.T) {
 	req := Request{Profile: testProfile(""), ProfileName: "default"}
 	cohort := ledger.ReviewerCohort{Adapter: "fake-llm", Members: []ledger.ReviewerCohortMember{{
@@ -7486,6 +7572,21 @@ func smallDiff(path string) string {
 	}, "\n")
 }
 
+func deletionDiff(path string) string {
+	return strings.Join([]string{
+		"diff --git a/" + path + " b/" + path,
+		"deleted file mode 100644",
+		"index 1111111..0000000",
+		"--- a/" + path,
+		"+++ /dev/null",
+		"@@ -1,3 +0,0 @@",
+		"-package main",
+		"-",
+		"-var changed = false",
+		"",
+	}, "\n")
+}
+
 func largeDiff(path, body string) string {
 	return strings.Join([]string{
 		"diff --git a/" + path + " b/" + path,

From 899ac2003940f58c32143a7a08606739b76dd16e Mon Sep 17 00:00:00 2001
From: Aaron Wong <6979793+zzwong@users.noreply.github.com>
Date: Thu, 10 Sep 2026 17:28:25 -0400
Subject: [PATCH 3/5] fix(review): Remove unused changed-file helper

---
 internal/pipeline/pipeline.go | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go
index 19b567ed..04eea607 100644
--- a/internal/pipeline/pipeline.go
+++ b/internal/pipeline/pipeline.go
@@ -3269,19 +3269,6 @@ func knownAgents(catalog agents.Catalog) map[string]bool {
 	return setBy(catalog.Agents, func(agent agents.Agent) string { return agent.ID })
 }
 
-func changedFiles(patches []FilePatch) map[string]bool {
-	paths := make([]string, 0, len(patches)*2)
-	for _, patch := range patches {
-		if patch.Path != "" {
-			paths = append(paths, patch.Path)
-		}
-		if patch.OldPath != "" {
-			paths = append(paths, patch.OldPath)
-		}
-	}
-	return stringSet(paths)
-}
-
 func knownThreads(threads []gitprovider.InlineThread) map[string]bool {
 	return threadIDSet(threads, func(thread gitprovider.InlineThread) gitprovider.ThreadID { return thread.ID })
 }

From 0a01493e8139a4542d5333abf8de1da3f04f39f6 Mon Sep 17 00:00:00 2001
From: Aaron Wong <6979793+zzwong@users.noreply.github.com>
Date: Fri, 11 Sep 2026 22:50:03 -0500
Subject: [PATCH 4/5] fix(review): correct deletion and rename handling in diff
 decode gates

Drop the fabricated "deleted file mode 100644" line from the reconstructed
GitHub diff. The pull request files payload carries no mode, so the hardcoded
value was concretely wrong for removed executables and symlinks. The parser
already classifies a removal from the "+++ /dev/null" header alone, which is
still emitted.

Widen the selection decode gate to accept rename source paths. The gate was
narrowed to reviewable head paths, but an orchestrator can legitimately cite a
"rename from" path that is plainly visible in the diff it was given, and an
unknown path fails the entire selection phase.

Separate the reviewer's obligation set from its acceptance set. Assignments,
prompt, and coverage stay on reviewable paths, but finding decode now accepts
every changed path so that a finding on a deleted file, or a deleted path in
skipped_files, no longer discards the reviewer's entire payload.
---
 internal/gitprovider/github/rest.go      |   1 -
 internal/gitprovider/github/rest_test.go |   7 +-
 internal/pipeline/diff_test.go           |   2 -
 internal/pipeline/pipeline.go            |  27 +++++-
 internal/pipeline/pipeline_test.go       | 107 +++++++++++++++++++++++
 5 files changed, 134 insertions(+), 10 deletions(-)

diff --git a/internal/gitprovider/github/rest.go b/internal/gitprovider/github/rest.go
index ec91bac6..89e649a5 100644
--- a/internal/gitprovider/github/rest.go
+++ b/internal/gitprovider/github/rest.go
@@ -186,7 +186,6 @@ func reconstructUnifiedDiff(files []pullFileResponse) string {
 			// deleted path.
 			switch f.Status {
 			case "removed":
-				fmt.Fprintf(&b, "deleted file mode 100644\n")
 				fmt.Fprintf(&b, "--- a/%s\n", oldPath)
 				b.WriteString("+++ /dev/null\n")
 			case "renamed":
diff --git a/internal/gitprovider/github/rest_test.go b/internal/gitprovider/github/rest_test.go
index 7d7be458..82ac50cc 100644
--- a/internal/gitprovider/github/rest_test.go
+++ b/internal/gitprovider/github/rest_test.go
@@ -470,10 +470,11 @@ func TestReconstructUnifiedDiff(t *testing.T) {
 		{"diff --git a/ro.go b/rn.go\nrename from ro.go\nrename to rn.go\n", true},
 		{"Binary files a/ro.go", false}, // a pure rename must not be marked binary
 		{"diff --git a/bin.dat b/bin.dat\nBinary files a/bin.dat and b/bin.dat differ\n", true},
-		{"diff --git a/removed.txt b/removed.txt\ndeleted file mode 100644\n--- a/removed.txt\n+++ /dev/null\n", true},
+		{"diff --git a/removed.txt b/removed.txt\n--- a/removed.txt\n+++ /dev/null\n", true},
 		{"diff --git a/removed.bin b/removed.bin\nBinary files a/removed.bin and b/removed.bin differ\n", false},
-		{"diff --git a/removed.bin b/removed.bin\ndeleted file mode 100644\n--- a/removed.bin\n+++ /dev/null\n", true},
-		{"+y", false}, // the empty-filename entry must be dropped entirely
+		{"diff --git a/removed.bin b/removed.bin\n--- a/removed.bin\n+++ /dev/null\n", true},
+		{"deleted file mode", false}, // no provider mode data: never fabricate one
+		{"+y", false},                // the empty-filename entry must be dropped entirely
 	}
 	for _, c := range cases {
 		if strings.Contains(got, c.substr) != c.want {
diff --git a/internal/pipeline/diff_test.go b/internal/pipeline/diff_test.go
index 82727535..15506c24 100644
--- a/internal/pipeline/diff_test.go
+++ b/internal/pipeline/diff_test.go
@@ -160,11 +160,9 @@ func TestParseUnifiedDiffRejectsBadHunkHeader(t *testing.T) {
 func TestParseUnifiedDiffPreservesPatchlessRemovedStatus(t *testing.T) {
 	raw := strings.Join([]string{
 		"diff --git a/removed.txt b/removed.txt",
-		"deleted file mode 100644",
 		"--- a/removed.txt",
 		"+++ /dev/null",
 		"diff --git a/removed.bin b/removed.bin",
-		"deleted file mode 100644",
 		"--- a/removed.bin",
 		"+++ /dev/null",
 		"",
diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go
index 04eea607..23f61d77 100644
--- a/internal/pipeline/pipeline.go
+++ b/internal/pipeline/pipeline.go
@@ -1259,6 +1259,9 @@ func runSelectionPhase(ctx context.Context, opts Options, req selectionPhaseRequ
 	// the head for a reviewer to inspect. Keep them out of the assignment
 	// contract and the post-selection backstops, matching buildReviewerCoverage.
 	reviewerFiles := reviewablePatchPaths(req.ParsedDiff.Patches)
+	// Decoding accepts more than it assigns: a rename's old path is visible in
+	// the diff, so citing it must not fail the whole selection.
+	selectableFiles := append(append([]string(nil), reviewerFiles...), renamedPatchOldPaths(req.ParsedDiff.Patches)...)
 	promptInput.ChangedFiles = append([]string(nil), reviewerFiles...)
 	dependencyTaskIDs := []string{dossier.SummaryTaskID}
 	fingerprintDeps := append(append([]string(nil), dependencyTaskIDs...), promptDeps...)
@@ -1276,7 +1279,7 @@ func runSelectionPhase(ctx context.Context, opts Options, req selectionPhaseRequ
 	decode := func(data []byte) (llm.Selection, error) {
 		return llm.DecodeSelection(data, llm.SelectionOptions{
 			KnownAgents:  knownAgents(req.Catalog),
-			ChangedFiles: stringSet(reviewerFiles),
+			ChangedFiles: stringSet(selectableFiles),
 			KnownThreads: knownThreadIDs,
 		})
 	}
@@ -2065,7 +2068,6 @@ func runReviewer(ctx context.Context, opts Options, req Request, runID string, p
 	model, effort := runtimeConfig.model, runtimeConfig.effort
 	changedFilePaths := reviewablePatchPaths(parsed.Patches)
 	selected = filterSelectedReviewerAssignment(selected, changedFilePaths)
-	assignmentScope := reviewerAssignmentScope(selected, changedFilePaths)
 	prompt, promptDeps, err := buildReviewerPrompt(artifacts, pr, selected, agent, changedFilePaths, resumeState.discussion)
 	if err != nil {
 		return llm.Findings{}, sessionDraft{}, ledger.Session{}, nil, Failure(FailureTerminal, err)
@@ -2115,8 +2117,10 @@ func runReviewer(ctx context.Context, opts Options, req Request, runID string, p
 		llmFailureStatus:  llmTaskStatusFailedIsolated,
 	}, func(data []byte) (llm.Findings, error) {
 		return llm.DecodeFindings(data, llm.FindingsOptions{
-			KnownAgents:  map[string]bool{agent.ID: true},
-			ChangedFiles: stringSet(assignmentScope),
+			KnownAgents: map[string]bool{agent.ID: true},
+			// Permitted subject matter is a superset of assigned work: a
+			// reviewer may report on a deleted path it was not assigned.
+			ChangedFiles: stringSet(patchPaths(parsed.Patches)),
 			NewFindingID: opts.newFindingID,
 		})
 	})
@@ -2630,6 +2634,21 @@ func deletedPatchPaths(patches []FilePatch) map[string]bool {
 	return deleted
 }
 
+// renamedPatchOldPaths returns the pre-rename paths still present in the diff.
+// They are not reviewer obligations, but an orchestrator may cite one because
+// the "rename from" header is visible in the dossier diff.
+func renamedPatchOldPaths(patches []FilePatch) []string {
+	paths := make([]string, 0, len(patches))
+	for _, patch := range patches {
+		if patch.Deleted || patch.OldPath == "" || patch.OldPath == patch.Path {
+			continue
+		}
+		paths = append(paths, patch.OldPath)
+	}
+	sort.Strings(paths)
+	return paths
+}
+
 // reviewablePatchPaths returns changed paths that a reviewer can inspect at
 // the head. Deleted paths remain in ParsedDiff and the dossier, but are not a
 // reviewer assignment or coverage obligation.
diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go
index 51180b1b..8df7f4ad 100644
--- a/internal/pipeline/pipeline_test.go
+++ b/internal/pipeline/pipeline_test.go
@@ -5436,6 +5436,103 @@ func TestDryRunDeletionOnlyDiffDoesNotRunSelectedReviewer(t *testing.T) {
 	}
 }
 
+func TestSelectionOnlyAcceptsRenameSourcePath(t *testing.T) {
+	ctx := context.Background()
+	provider, req := dryRunHarness(t)
+	removeRepoAgentFixture(provider)
+
+	gitCommandMustSucceed(t, provider.fixtureRepoDir, "mv", "main.go", "renamed.go")
+	gitCommandMustSucceed(t, provider.fixtureRepoDir, "commit", "-m", "rename main")
+	provider.pr.Head.SHA = gitCommandMustSucceed(t, provider.fixtureRepoDir, "rev-parse", "HEAD")
+	provider.diff.Raw = renameDiff("main.go", "renamed.go") + smallDiff("other.go")
+
+	selectionPayload := `{
+		"schema_version": 1,
+		"selected_agents": [{
+			"agent_id": "harness:reviewer",
+			"rationale": "the rename source is visible in the diff",
+			"files": ["main.go", "other.go"]
+		}],
+		"thread_actions": [],
+		"reasoning": "cite the rename source path"
+	}`
+	adapter := &llm.FakeAdapter{NameValue: "fake-llm"}
+	adapter.Queue(fakeLLMResult("selection-session", selectionPayload, 10, 2))
+	// A validation retry would consume this; the request count asserts it is unused.
+	adapter.Queue(fakeLLMResult("selection-session-retry", selectionPayload, 10, 2))
+
+	result, err := selectionOnlyForTest(ctx, Options{
+		Provider: provider,
+		Adapter:  adapter,
+		Now:      fixedNow,
+	}, selectionRequestFromReview(req, t.TempDir()))
+	if err != nil {
+		t.Fatalf("SelectionOnly: %v", err)
+	}
+	if len(adapter.Requests()) != 1 {
+		t.Fatalf("adapter requests = %d, want one selection request with no validation retry", len(adapter.Requests()))
+	}
+	if len(result.Selection.SelectedAgents) != 1 {
+		t.Fatalf("selected agents = %#v, want harness:reviewer", result.Selection.SelectedAgents)
+	}
+	// The rename source is not a reviewer obligation, so the assignment filter
+	// drops it and glob coverage backfills the head path.
+	if !reflect.DeepEqual(result.Selection.SelectedAgents[0].Files, []string{"other.go", "renamed.go"}) {
+		t.Fatalf("assigned files = %#v, want reviewable head paths", result.Selection.SelectedAgents[0].Files)
+	}
+}
+
+func TestDryRunReviewerFindingOnDeletedPathIsDecoded(t *testing.T) {
+	ctx := context.Background()
+	store := openPipelineStore(t)
+	defer closeStore(t, store)
+	provider, req := dryRunHarness(t)
+
+	gitCommandMustSucceed(t, provider.fixtureRepoDir, "rm", "main.go")
+	gitCommandMustSucceed(t, provider.fixtureRepoDir, "commit", "-m", "delete main")
+	provider.pr.Head.SHA = gitCommandMustSucceed(t, provider.fixtureRepoDir, "rev-parse", "HEAD")
+	provider.diff.Raw = deletionDiff("main.go") + smallDiff("other.go")
+
+	adapter := &llm.FakeAdapter{NameValue: "fake-llm"}
+	adapter.Queue(fakeLLMResult("selection-session", selectionJSON("harness:reviewer", "other.go"), 10, 2))
+	adapter.Queue(fakeLLMResult("reviewer-session", `{
+		"schema_version": 1,
+		"agent_id": "harness:reviewer",
+		"inspected_files": ["other.go"],
+		"skipped_files": ["main.go"],
+		"constraints": [],
+		"findings": [{
+			"severity": "major",
+			"file_path": "main.go",
+			"anchor": {"kind": "file"},
+			"body": "deleting main.go breaks other.go"
+		}]
+	}`, 20, 4))
+	adapter.Queue(fakeLLMResult("rollup-session", rollupJSON("comment", []string{"finding-1"}), 30, 6))
+
+	result, err := dryRunForTest(ctx, Options{
+		Provider:        provider,
+		Adapter:         adapter,
+		Store:           store,
+		Layout:          statepaths.NewLayout(t.TempDir(), t.TempDir()),
+		Now:             fixedNow,
+		NewRunID:        func() string { return "run-deleted-finding" },
+		NewSessionRowID: sequence("session"),
+		NewFindingID:    findingSequence("finding"),
+		NewActionID:     actionSequence(),
+		MaxConcurrency:  1,
+	}, req)
+	if err != nil {
+		t.Fatalf("DryRun: %v", err)
+	}
+	if len(result.ReviewerFailures) != 0 {
+		t.Fatalf("reviewer failures = %#v, want a decoded finding on the deleted path", result.ReviewerFailures)
+	}
+	if len(result.Findings) != 1 || result.Findings[0].FilePath != "main.go" {
+		t.Fatalf("findings = %#v, want one finding on the deleted main.go", result.Findings)
+	}
+}
+
 func TestRebaseReviewerCohortWithReviewablePathsExcludesDeletedFiles(t *testing.T) {
 	req := Request{Profile: testProfile(""), ProfileName: "default"}
 	cohort := ledger.ReviewerCohort{Adapter: "fake-llm", Members: []ledger.ReviewerCohortMember{{
@@ -7587,6 +7684,16 @@ func deletionDiff(path string) string {
 	}, "\n")
 }
 
+func renameDiff(oldPath, newPath string) string {
+	return strings.Join([]string{
+		"diff --git a/" + oldPath + " b/" + newPath,
+		"similarity index 100%",
+		"rename from " + oldPath,
+		"rename to " + newPath,
+		"",
+	}, "\n")
+}
+
 func largeDiff(path, body string) string {
 	return strings.Join([]string{
 		"diff --git a/" + path + " b/" + path,

From f7067e60da62af9727fe71a6bbaa4c13ce6889b8 Mon Sep 17 00:00:00 2001
From: Aaron Wong <6979793+zzwong@users.noreply.github.com>
Date: Fri, 11 Sep 2026 23:15:31 -0500
Subject: [PATCH 5/5] fix(review): keep finding decode inside the reviewer
 assignment

Accepting every changed path in the reviewer decode gate removed the only
enforcement of an assignment at the output boundary, so a reviewer scoped to a
subset could file findings on, and claim coverage for, files owned by another
reviewer or excluded from its workspace. Restore the assignment scope as the
acceptance base.

Express the widening once instead of twice. A single helper returns the paths a
model may cite but is never assigned - removed files and the pre-rename sources
still visible in the diff - and both the selection and finding gates union it
onto their obligation set, so the two cannot drift apart again.
---
 internal/pipeline/pipeline.go      |  34 ++++++++--
 internal/pipeline/pipeline_test.go | 103 +++++++++++++++++++++++++++++
 2 files changed, 130 insertions(+), 7 deletions(-)

diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go
index 23f61d77..8933adb7 100644
--- a/internal/pipeline/pipeline.go
+++ b/internal/pipeline/pipeline.go
@@ -1259,9 +1259,8 @@ func runSelectionPhase(ctx context.Context, opts Options, req selectionPhaseRequ
 	// the head for a reviewer to inspect. Keep them out of the assignment
 	// contract and the post-selection backstops, matching buildReviewerCoverage.
 	reviewerFiles := reviewablePatchPaths(req.ParsedDiff.Patches)
-	// Decoding accepts more than it assigns: a rename's old path is visible in
-	// the diff, so citing it must not fail the whole selection.
-	selectableFiles := append(append([]string(nil), reviewerFiles...), renamedPatchOldPaths(req.ParsedDiff.Patches)...)
+	// Citing a removed or pre-rename path must not fail the whole selection.
+	selectableFiles := append(append([]string(nil), reviewerFiles...), mentionableExtraPaths(req.ParsedDiff.Patches)...)
 	promptInput.ChangedFiles = append([]string(nil), reviewerFiles...)
 	dependencyTaskIDs := []string{dossier.SummaryTaskID}
 	fingerprintDeps := append(append([]string(nil), dependencyTaskIDs...), promptDeps...)
@@ -2068,6 +2067,8 @@ func runReviewer(ctx context.Context, opts Options, req Request, runID string, p
 	model, effort := runtimeConfig.model, runtimeConfig.effort
 	changedFilePaths := reviewablePatchPaths(parsed.Patches)
 	selected = filterSelectedReviewerAssignment(selected, changedFilePaths)
+	// A reviewer may cite its own assignment plus unassignable paths, nothing else.
+	citableFiles := append(append([]string(nil), reviewerAssignmentScope(selected, changedFilePaths)...), mentionableExtraPaths(parsed.Patches)...)
 	prompt, promptDeps, err := buildReviewerPrompt(artifacts, pr, selected, agent, changedFilePaths, resumeState.discussion)
 	if err != nil {
 		return llm.Findings{}, sessionDraft{}, ledger.Session{}, nil, Failure(FailureTerminal, err)
@@ -2117,10 +2118,8 @@ func runReviewer(ctx context.Context, opts Options, req Request, runID string, p
 		llmFailureStatus:  llmTaskStatusFailedIsolated,
 	}, func(data []byte) (llm.Findings, error) {
 		return llm.DecodeFindings(data, llm.FindingsOptions{
-			KnownAgents: map[string]bool{agent.ID: true},
-			// Permitted subject matter is a superset of assigned work: a
-			// reviewer may report on a deleted path it was not assigned.
-			ChangedFiles: stringSet(patchPaths(parsed.Patches)),
+			KnownAgents:  map[string]bool{agent.ID: true},
+			ChangedFiles: stringSet(citableFiles),
 			NewFindingID: opts.newFindingID,
 		})
 	})
@@ -2649,6 +2648,27 @@ func renamedPatchOldPaths(patches []FilePatch) []string {
 	return paths
 }
 
+// mentionableExtraPaths returns paths a model may cite but is never assigned:
+// removed files and the pre-rename sources still visible in the diff.
+func mentionableExtraPaths(patches []FilePatch) []string {
+	deleted := deletedPatchPaths(patches)
+	seen := make(map[string]bool, len(deleted))
+	paths := make([]string, 0, len(patches))
+	for path := range deleted {
+		seen[path] = true
+		paths = append(paths, path)
+	}
+	for _, path := range renamedPatchOldPaths(patches) {
+		if seen[path] {
+			continue
+		}
+		seen[path] = true
+		paths = append(paths, path)
+	}
+	sort.Strings(paths)
+	return paths
+}
+
 // reviewablePatchPaths returns changed paths that a reviewer can inspect at
 // the head. Deleted paths remain in ParsedDiff and the dossier, but are not a
 // reviewer assignment or coverage obligation.
diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go
index 8df7f4ad..72325db9 100644
--- a/internal/pipeline/pipeline_test.go
+++ b/internal/pipeline/pipeline_test.go
@@ -5533,6 +5533,109 @@ func TestDryRunReviewerFindingOnDeletedPathIsDecoded(t *testing.T) {
 	}
 }
 
+func TestDryRunReviewerFindingOnRenameSourcePathIsDecoded(t *testing.T) {
+	ctx := context.Background()
+	store := openPipelineStore(t)
+	defer closeStore(t, store)
+	provider, req := dryRunHarness(t)
+
+	gitCommandMustSucceed(t, provider.fixtureRepoDir, "mv", "main.go", "renamed.go")
+	gitCommandMustSucceed(t, provider.fixtureRepoDir, "commit", "-m", "rename main")
+	provider.pr.Head.SHA = gitCommandMustSucceed(t, provider.fixtureRepoDir, "rev-parse", "HEAD")
+	provider.diff.Raw = renameDiff("main.go", "renamed.go") + smallDiff("other.go")
+
+	adapter := &llm.FakeAdapter{NameValue: "fake-llm"}
+	adapter.Queue(fakeLLMResult("selection-session", selectionJSON("harness:reviewer", "renamed.go"), 10, 2))
+	adapter.Queue(fakeLLMResult("reviewer-session", `{
+		"schema_version": 1,
+		"agent_id": "harness:reviewer",
+		"inspected_files": ["renamed.go"],
+		"skipped_files": [],
+		"constraints": [],
+		"findings": [{
+			"severity": "major",
+			"file_path": "main.go",
+			"anchor": {"kind": "file"},
+			"body": "the rename drops a caller of main.go"
+		}]
+	}`, 20, 4))
+	adapter.Queue(fakeLLMResult("rollup-session", rollupJSON("comment", []string{"finding-1"}), 30, 6))
+
+	result, err := dryRunForTest(ctx, Options{
+		Provider:        provider,
+		Adapter:         adapter,
+		Store:           store,
+		Layout:          statepaths.NewLayout(t.TempDir(), t.TempDir()),
+		Now:             fixedNow,
+		NewRunID:        func() string { return "run-rename-source-finding" },
+		NewSessionRowID: sequence("session"),
+		NewFindingID:    findingSequence("finding"),
+		NewActionID:     actionSequence(),
+		MaxConcurrency:  1,
+	}, req)
+	if err != nil {
+		t.Fatalf("DryRun: %v", err)
+	}
+	if len(result.ReviewerFailures) != 0 {
+		t.Fatalf("reviewer failures = %#v, want a decoded finding on the rename source", result.ReviewerFailures)
+	}
+	if len(result.Findings) != 1 {
+		t.Fatalf("findings = %#v, want one finding anchored from the rename source", result.Findings)
+	}
+}
+
+func TestDryRunReviewerFindingOutsideAssignmentIsRejected(t *testing.T) {
+	ctx := context.Background()
+	store := openPipelineStore(t)
+	defer closeStore(t, store)
+	provider, req := dryRunHarness(t)
+	// The reviewer's **/*.go globs keep schema.sql out of its assignment, so the
+	// file belongs to whichever reviewer owns SQL, never to this one.
+	provider.diff.Raw = smallDiff("other.go") + smallDiff("schema.sql")
+
+	reviewerPayload := `{
+		"schema_version": 1,
+		"agent_id": "harness:reviewer",
+		"inspected_files": ["other.go"],
+		"skipped_files": [],
+		"constraints": [],
+		"findings": [{
+			"severity": "major",
+			"file_path": "schema.sql",
+			"anchor": {"kind": "file"},
+			"body": "schema.sql belongs to another reviewer"
+		}]
+	}`
+	adapter := &llm.FakeAdapter{NameValue: "fake-llm"}
+	adapter.Queue(fakeLLMResult("selection-session", selectionJSON("harness:reviewer", "other.go"), 10, 2))
+	adapter.Queue(fakeLLMResult("reviewer-session", reviewerPayload, 20, 4))
+	// The decode gate rejects the payload, so the reviewer gets one retry.
+	adapter.Queue(fakeLLMResult("reviewer-session-retry", reviewerPayload, 20, 4))
+	adapter.Queue(fakeLLMResult("rollup-session", rollupJSON("comment", []string{}), 30, 6))
+
+	result, err := dryRunForTest(ctx, Options{
+		Provider:        provider,
+		Adapter:         adapter,
+		Store:           store,
+		Layout:          statepaths.NewLayout(t.TempDir(), t.TempDir()),
+		Now:             fixedNow,
+		NewRunID:        func() string { return "run-unassigned-finding" },
+		NewSessionRowID: sequence("session"),
+		NewFindingID:    findingSequence("finding"),
+		NewActionID:     actionSequence(),
+		MaxConcurrency:  1,
+	}, req)
+	if err != nil {
+		t.Fatalf("DryRun: %v", err)
+	}
+	if len(result.Findings) != 0 {
+		t.Fatalf("findings = %#v, want none: schema.sql is outside the reviewer assignment", result.Findings)
+	}
+	if len(result.ReviewerFailures) != 1 || result.ReviewerFailures[0].AgentID != "harness:reviewer" {
+		t.Fatalf("reviewer failures = %#v, want the unassigned-path payload rejected", result.ReviewerFailures)
+	}
+}
+
 func TestRebaseReviewerCohortWithReviewablePathsExcludesDeletedFiles(t *testing.T) {
 	req := Request{Profile: testProfile(""), ProfileName: "default"}
 	cohort := ledger.ReviewerCohort{Adapter: "fake-llm", Members: []ledger.ReviewerCohortMember{{