Refuse a removal the pull-request body never names

.github/pull_request_template.md

@@ -13,7 +13,14 @@ under it, and say why the heading is gone rather than leaving a blank one.
 
 ## What this changes
 
-<!-- What the change does, in the words somebody reviewing it needs. -->
+<!--
+What the change does, in the words somebody reviewing it needs.
+
+A path this change takes out of the tree is named here, by path. Removing is
+allowed and removing in silence is not: the gate refuses a removed path this
+body never names, because a merge that deleted a licence, a guard and four
+hundred lines of a document once passed every check on this board.
+-->
 
 ## What failure it prevents
 

internal/pullrequest/pullrequest.go

@@ -359,6 +359,7 @@ func Judge(change Change) Verdict {
 	verdict.add(judgeExperiments(change))
 	verdict.add(judgeRecords(change))
 	verdict.add(judgeRemovals(change))
+	verdict.add(judgeRemovedPaths(change))
 	verdict.add(judgeSize(change))
 
 	return verdict

internal/pullrequest/pullrequest_test.go

@@ -79,7 +79,7 @@ func TestJudge(t *testing.T) {
 // to the rule they are about, and they are returned into this table rather than
 // run from a second harness.
 func judgeCases() []judgeCase {
-	return append(append(recordJudgeCases(), removalJudgeCases()...), []judgeCase{
+	return append(append(append(recordJudgeCases(), removalJudgeCases()...), removedPathJudgeCases()...), []judgeCase{
 		{
 			name:   "a change that names its issue and touches no experiment",
 			change: clean(),
@@ -216,6 +216,7 @@ func judgeCases() []judgeCase {
 			name: "an experiment's file removed without its record",
 			change: func() Change {
 				c := clean()
+				c.Body = "This closes #24. It removes experiments/one/measure.go under record 0004."
 				c.Files = []File{{Path: "experiments/one/measure.go", Gone: true}}
 				return c
 			}(),
@@ -292,6 +293,7 @@ func judgeCases() []judgeCase {
 				QuestionAlreadyAskedWasRewritten,
 				RecordAlreadyLandedWasRemoved,
 				ExperimentAlreadyLandedWasRenamed,
+				PathRemovedWithoutBeingNamed,
 				ChangeIsLargerThanOneReading,
 			},
 		},
@@ -315,6 +317,7 @@ func TestEveryPropertyHasACaseThatRefusesIt(t *testing.T) {
 		QuestionAlreadyAskedWasRewritten,
 		RecordAlreadyLandedWasRemoved,
 		ExperimentAlreadyLandedWasRenamed,
+		PathRemovedWithoutBeingNamed,
 	}
 
 	refused := make(map[string]bool)

internal/pullrequest/removal_test.go

@@ -24,6 +24,7 @@ func removalJudgeCases() []judgeCase {
 			name: "a record already on the branch that this change removes",
 			change: func() Change {
 				c := clean()
+				c.Body = "This closes #24. It removes " + record + " and experiments/one/measure.go."
 				c.Files = []File{
 					{Path: record, Gone: true},
 					{Path: "experiments/one/measure.go", Gone: true},
@@ -63,6 +64,7 @@ func removalJudgeCases() []judgeCase {
 			name: "a record this change removes that was never on the branch it lands on",
 			change: func() Change {
 				c := clean()
+				c.Body = "This closes #24. It removes " + record + ", which it also added."
 				c.Files = []File{{Path: record, Gone: true}}
 				c.Records = []RecordChange{{Path: record}}
 				return c
@@ -78,6 +80,7 @@ func removalJudgeCases() []judgeCase {
 			name: "an experiment's code removed with the record kept",
 			change: func() Change {
 				c := clean()
+				c.Body = "This closes #24. It removes experiments/one/measure.go under record 0004."
 				c.Files = []File{
 					{Path: "experiments/one/measure.go", Gone: true},
 					{Path: record},
@@ -107,6 +110,7 @@ func TestARemovedRecordNamesWhatMayBeRemovedInstead(t *testing.T) {
 	const record = "experiments/one/EXPERIMENT.md"
 
 	change := clean()
+	change.Body = "This closes #24. It removes " + record + "."
 	change.Files = []File{{Path: record, Gone: true}}
 	change.Records = []RecordChange{{
 		Path:          record,
@@ -201,6 +205,7 @@ func TestAMoveNothingReportedAsOneIsRefusedAsARemoval(t *testing.T) {
 	landed := recordAt("answered", theLandedAnswer)
 
 	change := clean()
+	change.Body = "This closes #24. It removes " + from + " and adds " + to + "."
 	change.Files = []File{{Path: from, Gone: true}, {Path: to}}
 	change.Records = []RecordChange{
 		{Path: from, Before: landed, BeforePresent: true},

internal/pullrequest/removedpath.go

@@ -0,0 +1,121 @@
+package pullrequest
+
+// The rule the incident in issue #155 asked for, which is about any tracked
+// path rather than about a record.
+//
+// One merge on the default branch removed landed work on seven paths under a
+// message describing a change to how one workflow pin is commented. A licence,
+// a guard and four hundred lines of a document went, nothing was red, and the
+// suite stayed green afterwards because almost everything removed was prose and
+// the one test that went with it was the only thing reading the code path it
+// covered.
+//
+// WHAT EVERY OTHER RULE HERE READS IS WHAT THE TREE BECAME. The record checks
+// walk experiments/, the invariants read tracked text, the formatting rules read
+// the bytes around the words, and a merge whose result compiles and passes says
+// nothing about what it replaced. This package holds both ends of the range, so
+// it is the only place a removal is visible at all.
+//
+// REMOVING STAYS ALLOWED. What ends is removing in silence. The refusal is
+// against a body that does not mention the path, and the repair is one sentence
+// in the body rather than putting the file back.
+
+import (
+	"fmt"
+	"strings"
+)
+
+// PathRemovedWithoutBeingNamed refuses a change that takes a tracked path out
+// of the tree without naming that path in the pull-request body.
+//
+// The failure is what the incident actually looked like rather than a
+// hypothetical. Nobody decided to delete a licence. A branch was cut from an
+// older state of the default branch and pushed on top of a newer one, the merge
+// replaced seven paths with what they had said earlier, and the body of the
+// change described something else entirely because that is what its author
+// believed it did. A reader had no line to disagree with.
+//
+// What the refusal converts is exactly that: a removal nobody wrote down
+// becomes a removal somebody had to write down. It cannot ask whether the
+// sentence is true, and the residual is written at judgeRemovedPaths below.
+const PathRemovedWithoutBeingNamed = "path-removed-without-being-named"
+
+// judgeRemovedPaths holds every path the range takes out of the tree to being
+// named in the pull-request body.
+//
+// THE BOUNDARIES, WRITTEN HERE RATHER THAN DISCOVERED.
+//
+// It reads whether the body carries the path, and never whether what the body
+// says about it is true or even that it is about the removal at all. A body
+// that names the path while claiming the opposite passes, and so does one that
+// names it for an unrelated reason. That bound is the same one the issue rule
+// above carries: what a rule reading a string can separate is the case where
+// nothing was written from the case where something was, and a reviewer is what
+// stands behind the rest. It is worth having anyway, because the case where
+// nothing was written is the case that happened.
+//
+// The source path of a move is not judged. git reports a rename as one entry
+// carrying both paths, so the change says where the content went and a reader
+// of the diff is not left guessing, which is the thing this rule is against.
+// Where a move is not reported as one, and a rename made together with a
+// rewrite is where it will not be, the old path is an ordinary removal here and
+// has to be named. That is red for a reason a reader can act on rather than
+// silently narrower than it looks.
+//
+// A path added and removed inside one branch never reaches this rule, because
+// the range is read from base to head and the file is not in that diff at all.
+//
+// WHAT IT CANNOT SEE is a removal that arrives without passing this gate: a
+// direct push, an edit made through the web interface, or history rewritten on
+// the branch. The first is refused by the ruleset on the default branch, which
+// requires a pull request, and the others are named here so that a green run is
+// not read as covering them.
+func judgeRemovedPaths(change Change) Verdict {
+	if !change.FilesRead {
+		return Verdict{Skips: []Skip{{
+			Rule: PathRemovedWithoutBeingNamed,
+			Why:  "this run was given no changed paths, so nothing was read that could have been removed",
+		}}}
+	}
+	if !change.BodyRead {
+		return Verdict{Skips: []Skip{{
+			Rule: PathRemovedWithoutBeingNamed,
+			Why:  "this run was given no pull-request body, so nothing was read to look for a removed path in",
+		}}}
+	}
+
+	moved := renames(change)
+	var verdict Verdict
+	for _, file := range change.Files {
+		if !file.Gone {
+			continue
+		}
+		if _, isMove := moved[file.Path]; isMove {
+			continue
+		}
+		if namesPath(change.Body, file.Path) {
+			continue
+		}
+		verdict.Refusals = append(verdict.Refusals, Refusal{
+			Property: PathRemovedWithoutBeingNamed,
+			Subject:  file.Path,
+			Detail:   fmt.Sprintf("this change takes it out of the tree and the pull-request body never names it, so a reader has nothing to disagree with. Removing stays allowed: name %s in the body and say why it goes", file.Path),
+		})
+	}
+	return verdict
+}
+
+// namesPath says whether a body carries a path.
+//
+// The comparison is a plain substring and that is a decision rather than the
+// easy thing. A path written inside a link, inside backticks, at the end of a
+// sentence or inside a longer path all carry the same bytes, and a rule that
+// tried to be cleverer about the surroundings would refuse a body that names
+// the file perfectly well. What it costs is that a body naming a longer path
+// which happens to end in this one satisfies the rule for both, which is a
+// direction that lets honest work through rather than one that hides a removal:
+// the longer path is the file beside it, and a reader who reaches either
+// sentence is reading about the directory that went.
+func namesPath(body, path string) bool {
+	return path != "" && strings.Contains(body, path)
+}

internal/pullrequest/removedpath_test.go

@@ -0,0 +1,160 @@
+package pullrequest
+
+import (
+	"strings"
+	"testing"
+)
+
+// The cases over a path the range takes out of the tree. They are returned into
+// the one table in pullrequest_test.go rather than run from a second harness, so
+// the proof that every property has a case reads them too.
+//
+// Every case here is one change away from a case that refuses nothing, and the
+// one change is a sentence in the body rather than a file put back. That is the
+// whole shape of the rule: removing stays allowed, removing in silence does not.
+func removedPathJudgeCases() []judgeCase {
+	const gone = "docs/quality-parity.md"
+	const alsoGone = "internal/contexts/contexts_test.go"
+
+	return []judgeCase{
+		{
+			name: "a change that removes a tracked path and never names it",
+			change: func() Change {
+				c := clean()
+				c.Files = []File{
+					{Path: ".github/workflows/zizmor.yml"},
+					{Path: gone, Gone: true},
+				}
+				return c
+			}(),
+			// The incident this rule comes from, reduced to one path. The body
+			// described something else, which is what its author believed the
+			// change did, and a reader had no line to disagree with.
+			want: []string{PathRemovedWithoutBeingNamed},
+		},
+		{
+			name: "a change that removes a tracked path and names it in the body",
+			change: func() Change {
+				c := clean()
+				c.Body = "This closes #24. It removes " + gone + ", whose two sections moved into the operator guide."
+				c.Files = []File{
+					{Path: ".github/workflows/zizmor.yml"},
+					{Path: gone, Gone: true},
+				}
+				return c
+			}(),
+			// The near miss, and the one field that separates it from the case
+			// above is the body. Nothing about the diff changed.
+		},
+		{
+			name: "a change that removes two paths and names only one",
+			change: func() Change {
+				c := clean()
+				c.Body = "This closes #24. It removes " + gone + " because the document moved."
+				c.Files = []File{
+					{Path: gone, Gone: true},
+					{Path: alsoGone, Gone: true},
+				}
+				return c
+			}(),
+			// One refusal per path rather than one for the range, because the
+			// repair is a sentence about a particular file and a reader given
+			// one line saying something was removed has to go and find which.
+			want: []string{PathRemovedWithoutBeingNamed},
+		},
+		{
+			name: "a path that git reported as moved",
+			change: func() Change {
+				c := clean()
+				c.Files = []File{
+					{Path: "docs/parity.md", Gone: true},
+					{Path: gone, From: "docs/parity.md"},
+				}
+				return c
+			}(),
+			// The boundary. The entry carries both paths, so the change says
+			// where the content went and the diff is not silent about it, which
+			// is the thing this rule is against.
+		},
+		{
+			name: "a rename nothing reported as one",
+			change: func() Change {
+				c := clean()
+				c.Files = []File{
+					{Path: "docs/parity.md", Gone: true},
+					{Path: gone},
+				}
+				return c
+			}(),
+			// The residual named at the rule, held in a case rather than only
+			// in a comment. Where a move is not reported as one, and a rename
+			// made together with a rewrite is where it will not be, the old
+			// path is an ordinary removal here and has to be named.
+			want: []string{PathRemovedWithoutBeingNamed},
+		},
+		{
+			name: "a change that reads no body and removes a path",
+			change: func() Change {
+				c := clean()
+				c.Body = ""
+				c.BodyRead = false
+				c.Files = []File{{Path: gone, Gone: true}}
+				return c
+			}(),
+			// A run that read no body has not seen a removal go unnamed, and
+			// saying so is different from passing. The issue rule skips on the
+			// same input for the same reason.
+			skips: []string{BodyNamesNoIssue, PathRemovedWithoutBeingNamed},
+		},
+	}
+}
+
+// TestARemovalRefusalNamesThePathAndTheRepair holds the message to the two
+// things whoever hit it needs. Which file went, because a body has to name it
+// and the author is about to type it; and that the repair is a sentence rather
+// than putting the file back, because a rule that reads as forbidding removals
+// is a rule somebody argues with instead of satisfying.
+func TestARemovalRefusalNamesThePathAndTheRepair(t *testing.T) {
+	const gone = "docs/quality-parity.md"
+	change := clean()
+	change.Files = []File{{Path: gone, Gone: true}}
+
+	verdict := Judge(change)
+	if len(verdict.Refusals) != 1 {
+		t.Fatalf("expected one refusal, got %d", len(verdict.Refusals))
+	}
+	refusal := verdict.Refusals[0]
+	if refusal.Property != PathRemovedWithoutBeingNamed {
+		t.Fatalf("expected %s, got %s", PathRemovedWithoutBeingNamed, refusal.Property)
+	}
+	if refusal.Subject != gone {
+		t.Errorf("the refusal names %q rather than the path that went", refusal.Subject)
+	}
+	if !strings.Contains(refusal.Detail, gone) {
+		t.Errorf("the detail never names the path the author has to type: %q", refusal.Detail)
+	}
+	if !strings.Contains(refusal.Detail, "Removing stays allowed") {
+		t.Errorf("the detail does not say the repair is a sentence rather than putting the file back: %q", refusal.Detail)
+	}
+}
+
+// TestNamingOneRemovedPathDoesNotCoverAnother is the near miss that a rule
+// written as "the body mentions a removal" would pass. A body saying one file
+// went is not a body saying two did, and the failure this rule is against is
+// exactly a change that removed more than its author knew about.
+func TestNamingOneRemovedPathDoesNotCoverAnother(t *testing.T) {
+	const named = "docs/quality-parity.md"
+	const unnamed = "LICENSE"
+
+	change := clean()
+	change.Body = "This closes #24 and removes " + named + "."
+	change.Files = []File{{Path: named, Gone: true}, {Path: unnamed, Gone: true}}
+
+	verdict := Judge(change)
+	if len(verdict.Refusals) != 1 {
+		t.Fatalf("expected exactly one refusal, got %d", len(verdict.Refusals))
+	}
+	if verdict.Refusals[0].Subject != unnamed {
+		t.Errorf("the refusal names %q rather than the path the body left out", verdict.Refusals[0].Subject)
+	}
+}