🐛 fix: unblock review backlog findings in untouched files

changelog.d/fixed-14473-review-backlog-scope.md

@@ -0,0 +1 @@
+- Review backlog findings in files untouched by the reviewed PR are actionable immediately instead of waiting for that PR to close or merge.

src/pkg/github/issue_blocked_test.go

@@ -129,9 +129,8 @@ func TestBlockedBacklogLabels(t *testing.T) {
 
 // hivecommons/hive#13093: a finding whose file the PR introduces (absent on
 // the base branch) is filed blocked — marker in the body, `blocked` label,
-// no `help wanted` — while a finding on a file the base already has files
-// as before.
-func TestReviewBacklogFilesEveryItemBlockedOnItsPR(t *testing.T) {
+// no `help wanted` — as is a finding on an existing file changed by the PR.
+func TestReviewBacklogFilesIntroducedAndTouchedItemsBlockedOnItsPR(t *testing.T) {
 	withReviewBacklogState(t)
 	var issues []struct {
 		Body   string   `json:"body"`
@@ -144,6 +143,8 @@ func TestReviewBacklogFilesEveryItemBlockedOnItsPR(t *testing.T) {
 			_, _ = io.WriteString(w, `[]`)
 		case r.Method == "GET" && r.URL.Path == "/repos/o/r/pulls/5":
 			_, _ = io.WriteString(w, `{"number":5,"state":"open","base":{"ref":"v5"}}`)
+		case r.Method == "GET" && r.URL.Path == "/repos/o/r/pulls/5/files":
+			_, _ = io.WriteString(w, `[{"filename":"pkg/old.go","status":"modified"}]`)
 		case r.Method == "GET" && r.URL.Path == "/repos/o/r/contents/pkg/new.go":
 			if r.URL.Query().Get("ref") != "v5" {
 				t.Errorf("contents probe ref = %q, want v5", r.URL.Query().Get("ref"))
@@ -191,9 +192,8 @@ func TestReviewBacklogFilesEveryItemBlockedOnItsPR(t *testing.T) {
 	if len(issues) != 2 {
 		t.Fatalf("issues = %d, want 2", len(issues))
 	}
-	// Every item is parked on the reviewed PR (#13833); only the note's
-	// reason differs between a file the PR introduces and one already on
-	// the base branch.
+	// Introduced and touched files are parked on the reviewed PR; the note's
+	// reason distinguishes a missing base file from a changed existing file.
 	var missing, onBase int
 	for _, is := range issues {
 		if _, pr, ok := ParseBlockedOnMarker(is.Body); !ok || pr != 5 {

src/pkg/github/review_backlog.go

@@ -193,10 +193,9 @@ func (l *reviewBacklogLedger) save(path string, now time.Time) error {
 // updated counts toward the per-PR daily cap, and each pass that wrote
 // anything leaves one batch audit entry.
 //
-// Every item filed from a review of an open PR is parked on that PR
-// (hivecommons/hive#13833): a below-line or out-of-scope finding is almost
-// always about lines the PR changes, so fixing it on the base branch before
-// the PR lands is wrong or conflicts. ReconcileReviewBacklogBlocks lifts the
+// Findings in files changed by the PR or missing on its base are parked on
+// that PR; findings in untouched files can be worked immediately (#14473).
+// ReconcileReviewBacklogBlocks lifts the
 // block when the PR merges, and unblocks the item for re-triage (never closes
 // it) when the PR closes unmerged.
 func (c *Client) fileOutOfScopeReviewBacklog(ctx context.Context, req ReviewRequest, now time.Time) error {
@@ -242,6 +241,7 @@ func (c *Client) fileReviewBacklog(ctx context.Context, req ReviewRequest, now t
 	// hivecommons/hive#13093: a finding whose evidence file the PR introduces
 	// is parked on the PR, not offered as work.
 	baseProbe := c.newReviewBacklogBaseProbe(req.Repo, req.Number)
+	changedFiles := c.newReviewBacklogChangedFilesProbe(req.Repo, req.Number)
 	var filedIssues []reviewBacklogFiled
 	// listed is every item this pass's findings map to — filed, re-raised
 	// onto an existing item, or already tracked for this PR — for the
@@ -318,16 +318,25 @@ func (c *Client) fileReviewBacklog(ctx context.Context, req ReviewRequest, now t
 				continue
 			}
 			title := reviewBacklogIssueTitle(report.Perspective, finding)
-			// Every item is parked on the reviewed PR (#13833). The note
-			// says why: the cited file only exists in the PR (#13093), or
-			// the finding is about code the PR is changing.
-			blocked := true
-			itemLabels := blockedBacklogLabels(labels)
+			missing := baseProbe.FileMissingOnBase(ctx, finding.File)
+			touched, filesKnown := false, true
+			if !missing {
+				touched, filesKnown = changedFiles(ctx, finding.File)
+			}
+			blocked := missing || touched || !filesKnown
+			itemLabels := labels
+			if blocked {
+				itemLabels = blockedBacklogLabels(labels)
+			}
 			body := c.reviewBacklogIssueBody(req, report.Perspective, finding, headSHA, routing.Severity.BlockAt, fp)
-			if baseProbe.FileMissingOnBase(ctx, finding.File) {
+			if missing {
 				body = blockedBacklogNote(req.Repo, req.Number, finding.File, baseProbe.BaseRef(ctx)) + body
-			} else {
-				body = blockedOnReviewedPRNote(req.Repo, req.Number) + body
+			} else if blocked {
+				note := blockedOnReviewedPRNote(req.Repo, req.Number)
+				if !filesKnown {
+					note = strings.Replace(note, "is about code that PR is changing", "could not be checked against the PR's changed files", 1)
+				}
+				body = note + body
 			}
 			ref, err := sink.CreateItem(ctx, ReviewBacklogItem{
 				Repo:        req.Repo,
@@ -384,10 +393,12 @@ func (c *Client) fileReviewBacklog(ctx context.Context, req ReviewRequest, now t
 			}
 			if !ref.Reused {
 				// A filing folded into an open twin keeps the twin's own
-				// state (#13364); only an item this pass created waits on
-				// the PR.
+				// state (#13364). Keep source provenance for new items,
+				// but only park findings related to the PR.
 				fpRecord.BlockedOnPR = req.Number
-				fpRecord.BlockState = reviewBacklogBlockBlocked
+				if blocked {
+					fpRecord.BlockState = reviewBacklogBlockBlocked
+				}
 			}
 			ledger.setFingerprint(req.Repo, fp, fpRecord)
 			if outOfScope {

src/pkg/github/review_backlog_blocks.go

@@ -12,14 +12,11 @@ import (
 	gh "github.com/google/go-github/v72/github"
 )
 
-// Review backlog items are parked on the PR they were filed from
-// (hivecommons/hive#13833). A below-line or out-of-scope finding is almost
-// always about lines the PR itself changes, so working it on the base branch
-// before the PR lands is wrong or conflicts. Every item the review backlog
-// creates carries the blocked-on marker and the `blocked` label, and the
-// ledger records the PR it waits on. ReconcileReviewBacklogBlocks, run from
-// the governor every cycle, resolves the block in the item's own destination
-// (GitHub issue, Linear, Jira, GitHub Projects):
+// Review backlog items in files changed by the reviewed PR (or missing on
+// its base) wait for that PR (#13833, #14473). Untouched-file findings are
+// immediately actionable. Failed changed-file lookups conservatively keep
+// the block. ReconcileReviewBacklogBlocks resolves parked items in their own
+// destination (GitHub issue, Linear, Jira, GitHub Projects):
 //
 //   - the PR merged: the block is lifted with a one-line note telling the
 //     worker to check the finding still applies and to close the item without
@@ -351,7 +348,7 @@ func reviewFollowUpsSection(listed []reviewBacklogFiled, capped int) string {
 	var b strings.Builder
 	b.WriteString(reviewFollowUpsMarker)
 	b.WriteString("\n### Follow-ups filed\n")
-	b.WriteString("Non-blocking findings from this review are tracked as backlog items, so they are not filed again. New items wait on this PR: fix them here, or leave them and they unblock when it merges (if it closes unmerged they are unblocked for re-triage, not closed).\n\n")
+	b.WriteString("Non-blocking findings from this review are tracked as backlog items, so they are not filed again. Items in files changed by this PR wait on it: fix them here, or leave them and they unblock when it merges (if it closes unmerged they are unblocked for re-triage, not closed). Findings in untouched files are actionable immediately.\n\n")
 	seen := map[string]bool{}
 	for _, it := range sorted {
 		ref := it.key
@@ -377,3 +374,47 @@ func reviewFollowUpsSection(listed []reviewBacklogFiled, capped int) string {
 	}
 	return b.String()
 }
+
+// newReviewBacklogChangedFilesProbe caches a bounded, paginated lookup per
+// filing pass. A partial list cannot prove a file untouched, so errors and
+// exhausted budgets report unknown rather than releasing potentially blocked work.
+// Paths are normalised on both sides so a finding citing "./pkg/a.go" or
+// "/pkg/a.go" still matches GitHub's repo-root-relative "pkg/a.go".
+func (c *Client) newReviewBacklogChangedFilesProbe(repo string, pr int) func(context.Context, string) (bool, bool) {
+	loaded, complete := false, false
+	files := map[string]bool{}
+	return func(ctx context.Context, file string) (bool, bool) {
+		if !loaded {
+			loaded = true
+			owner, name := c.splitRepo(repo)
+			opts := &gh.ListOptions{PerPage: 100}
+			count := 0
+			for n := 0; n < reviewBacklogBlockLookups; n++ {
+				changed, resp, err := c.client.PullRequests.ListFiles(ctx, owner, name, pr, opts)
+				if err != nil {
+					break
+				}
+				count += len(changed)
+				for _, f := range changed {
+					if p := normaliseFingerprintPath(f.GetFilename()); p != "" {
+						files[p] = true
+					}
+					if old := normaliseFingerprintPath(f.GetPreviousFilename()); old != "" {
+						files[old] = true
+					}
+				}
+				if resp.NextPage == 0 {
+					// GitHub caps this endpoint at 3000 files.
+					complete = count < 3000
+					break
+				}
+				opts.Page = resp.NextPage
+			}
+		}
+		file = normaliseFingerprintPath(file)
+		if files[file] {
+			return true, true
+		}
+		return false, complete && file != ""
+	}
+}

src/pkg/github/review_backlog_scope_test.go

@@ -0,0 +1,129 @@
+package github
+
+import (
+	"context"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"path"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/hivecommons/hive/pkg/config"
+)
+
+func TestReviewBacklogBlocksOnlyRelatedFiles(t *testing.T) {
+	for _, tc := range []struct {
+		name, files, file        string
+		missing, failed, blocked bool
+	}{
+		{name: "untouched", files: `[{"filename":"other.go"}]`},
+		{name: "changed", files: `[{"filename":"pkg/a.go"}]`, blocked: true},
+		{name: "dot-relative finding path", files: `[{"filename":"pkg/a.go"}]`, file: "./pkg/a.go", blocked: true},
+		{name: "root-relative finding path", files: `[{"filename":"pkg/a.go"}]`, file: "/pkg/a.go", blocked: true},
+		{name: "renamed", files: `[{"filename":"new.go","previous_filename":"pkg/a.go"}]`, blocked: true},
+		{name: "missing on base", missing: true, blocked: true},
+		{name: "lookup failed", failed: true, blocked: true},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			withReviewBacklogState(t)
+			file := tc.file
+			if file == "" {
+				file = "pkg/a.go"
+			}
+			lookups := 0
+			srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				w.Header().Set("Content-Type", "application/json")
+				// Clean so a "./pkg/a.go" finding still resolves the base probe's contents lookup.
+				switch path.Clean(r.URL.Path) {
+				case "/repos/o/r/issues/5/comments":
+					fmt.Fprint(w, `{"id":1}`)
+				case "/repos/o/r/pulls/5":
+					fmt.Fprint(w, `{"base":{"ref":"v5"}}`)
+				case "/repos/o/r/contents/pkg/a.go":
+					if tc.missing {
+						w.WriteHeader(http.StatusNotFound)
+						return
+					}
+					fmt.Fprint(w, `{"type":"file","path":"pkg/a.go"}`)
+				case "/repos/o/r/pulls/5/files":
+					lookups++
+					if tc.failed {
+						w.WriteHeader(http.StatusInternalServerError)
+						return
+					}
+					fmt.Fprint(w, tc.files)
+				default:
+					w.WriteHeader(http.StatusNotFound)
+				}
+			}))
+			defer srv.Close()
+			sink := &fakeBacklogSink{dest: config.ReviewBacklogLinear}
+			c := headsBacklogClient(t, srv.URL, sink)
+			report := routingReport(t, 5, "abc", nit("first", "low", file, 7, "evidence"), nit("second", "low", file, 8, "other evidence"))
+			if err := c.fileOutOfScopeReviewBacklog(context.Background(), ReviewRequest{Repo: "o/r", Number: 5, Agent: "reviewer", Report: report}, time.Now()); err != nil {
+				t.Fatal(err)
+			}
+			if len(sink.created) != 2 {
+				t.Fatalf("created %d items", len(sink.created))
+			}
+			for _, item := range sink.created {
+				if got := strings.Contains(strings.Join(item.Labels, ","), BlockedLabel); got != tc.blocked {
+					t.Fatalf("labels %v, blocked want %v", item.Labels, tc.blocked)
+				}
+				if got := strings.Contains(item.Body, BlockedOnMarker("o/r", 5)); got != tc.blocked {
+					t.Fatalf("block marker = %v, want %v", got, tc.blocked)
+				}
+				if tc.failed && strings.Contains(item.Body, "is about code that PR is changing") {
+					t.Fatal("unknown file asserted changed")
+				}
+			}
+			wantLookups := 1
+			if tc.missing {
+				wantLookups = 0
+			}
+			if lookups != wantLookups {
+				t.Fatalf("lookups %d, want %d", lookups, wantLookups)
+			}
+			ledger, err := loadReviewBacklog("")
+			if err != nil {
+				t.Fatal(err)
+			}
+			for _, rec := range ledger.Fingerprints[reviewBacklogRepoKey("o/r")] {
+				if rec.BlockedOnPR != 5 {
+					t.Fatal("lost source PR provenance")
+				}
+				if got := rec.BlockState == reviewBacklogBlockBlocked; got != tc.blocked {
+					t.Fatalf("block state %q", rec.BlockState)
+				}
+			}
+		})
+	}
+}
+
+func TestReviewBacklogChangedFilesPagination(t *testing.T) {
+	requests := 0
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		requests++
+		w.Header().Set("Content-Type", "application/json")
+		if r.URL.Query().Get("page") == "2" {
+			fmt.Fprint(w, `[{"filename":"second.go"}]`)
+			return
+		}
+		w.Header().Set("Link", fmt.Sprintf(`<http://%s/repos/o/r/pulls/5/files?page=2>; rel="next"`, r.Host))
+		fmt.Fprint(w, `[{"filename":"first.go"}]`)
+	}))
+	defer srv.Close()
+	c := reviewTestClient(t, srv.URL)
+	probe := c.newReviewBacklogChangedFilesProbe("o/r", 5)
+	if changed, known := probe(context.Background(), "second.go"); !changed || !known {
+		t.Fatal("missed second page")
+	}
+	if changed, known := probe(context.Background(), "untouched.go"); changed || !known {
+		t.Fatal("untouched file not known")
+	}
+	if requests != 2 {
+		t.Fatalf("requests = %d", requests)
+	}
+}