From e02c06e32d34605bfd414a4bafc48624e6ef227d Mon Sep 17 00:00:00 2001 From: Nils Lehnen <30603423+iderex@users.noreply.github.com> Date: Wed, 12 Aug 2026 03:15:27 +0200 Subject: [PATCH] Refuse a record that is removed or renamed after it lands (#69) The decision that records are permanent says a record is not deleted and is not rewritten to hide what it said. Only the second half was refused. A change that removed an EXPERIMENT.md, or the directory it sits in, passed every check here, because the runner walks the tree and a record that is gone is not a record it can meet. So the rule this board's central claim rests on held against editing and not against deleting, and deleting is the cheaper way to make an experiment stop having existed. Two refusals in the deterministic pull-request check, next to the answer rule they complete, because telling a removal from a record that was never there needs the version at the base of the range and this check already holds both ends of it. A record present at the base and absent at the head is refused, and the message names what record 0004 does allow: the code goes, the record stays, and it gains the line naming the commit that removed the code. A move is refused separately and the message names both ends of it, because the repair is a choice the check may not make between keeping the slug and writing a new experiment that says where the work went. Which of the two a change is comes from what git reported, so the diff is read with --find-renames rather than with whatever diff.renames is set to on the machine that runs it, and where a move is not reported as one it is refused as a removal, which is stated at the rule. Two boundaries are written at the rule. A record created and removed inside one branch is not covered, because nothing about it reached the branch the change lands on. History being rewritten on the branch itself is the ruleset's refusal rather than this one, and saying so is what keeps a green run from being read as covering it. ParseFiles now carries the pairing git prints for a rename. Deleting it left every case green, because the judgement is proved against values and the parser against what git prints, and neither half saw the field being dropped between them. The case that closes that join was added after the deletion showed the gap. Signed-off-by: Nils Lehnen <30603423+iderex@users.noreply.github.com> --- cmd/pullrequest/main.go | 7 +- internal/pullrequest/pullrequest.go | 14 +- internal/pullrequest/pullrequest_test.go | 6 +- internal/pullrequest/read.go | 7 +- internal/pullrequest/read_test.go | 2 +- internal/pullrequest/record.go | 3 +- internal/pullrequest/removal.go | 134 ++++++++++++++ internal/pullrequest/removal_test.go | 214 +++++++++++++++++++++++ 8 files changed, 379 insertions(+), 8 deletions(-) create mode 100644 internal/pullrequest/removal.go create mode 100644 internal/pullrequest/removal_test.go diff --git a/cmd/pullrequest/main.go b/cmd/pullrequest/main.go index eb51a98..df807c0 100644 --- a/cmd/pullrequest/main.go +++ b/cmd/pullrequest/main.go @@ -138,7 +138,12 @@ func readChange(event pullrequest.Event, git func(args ...string) ([]byte, error // commit landing on the base is not this pull request's change. span := event.Base + "..." + event.Head - out, err := git("diff", "--name-status", "-z", span) + // Rename detection is asked for rather than relied on. git turns it on by + // default, so leaving it out reads the same on almost every machine and + // differently on one that set diff.renames off, and the rule over a + // renamed experiment would then refuse a move as a removal for a reason + // living in somebody's git config. + out, err := git("diff", "--name-status", "--find-renames", "-z", span) if err != nil { return change, err } diff --git a/internal/pullrequest/pullrequest.go b/internal/pullrequest/pullrequest.go index 3b84381..2e1ffa2 100644 --- a/internal/pullrequest/pullrequest.go +++ b/internal/pullrequest/pullrequest.go @@ -200,6 +200,15 @@ type File struct { // record rule treats a removal as touching the experiment, for the reason // written at that property. Gone bool + + // From is where this file was moved from, and it is empty on every file + // that was not moved. git reports a rename as one entry carrying two + // paths, and the pairing between them is the whole of what the entry adds + // over a removal beside an addition. Dropping it costs the rule over a + // renamed experiment the second path its refusal has to name, and a + // refusal that names only where a record used to be sends the reader + // looking for where it went. + From string } // A Commit is one commit in the range, as much of it as any rule here reads. @@ -349,6 +358,7 @@ func Judge(change Change) Verdict { verdict.add(judgeCommits(change)) verdict.add(judgeExperiments(change)) verdict.add(judgeRecords(change)) + verdict.add(judgeRemovals(change)) verdict.add(judgeSize(change)) return verdict @@ -418,8 +428,8 @@ func judgeCommits(change Change) Verdict { // A change that removes an experiment's record along with its files satisfies // this rule, because the record is in the set of paths the change touched. That // a landed record may not be removed at all is a different rule about a -// different failure, and it is issue #69's rather than this one's. Nothing here -// should be read as permitting it. +// different failure, and it is record-already-landed-was-removed rather than +// this one. Nothing here should be read as permitting it. func judgeExperiments(change Change) Verdict { if !change.FilesRead { return Verdict{Skips: []Skip{{ diff --git a/internal/pullrequest/pullrequest_test.go b/internal/pullrequest/pullrequest_test.go index 4790a17..5b0fd9b 100644 --- a/internal/pullrequest/pullrequest_test.go +++ b/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(recordJudgeCases(), []judgeCase{ + return append(append(recordJudgeCases(), removalJudgeCases()...), []judgeCase{ { name: "a change that names its issue and touches no experiment", change: clean(), @@ -290,6 +290,8 @@ func judgeCases() []judgeCase { ExperimentChangedWithoutItsRecord, AnswerAlreadyLandedWasRewritten, QuestionAlreadyAskedWasRewritten, + RecordAlreadyLandedWasRemoved, + ExperimentAlreadyLandedWasRenamed, ChangeIsLargerThanOneReading, }, }, @@ -311,6 +313,8 @@ func TestEveryPropertyHasACaseThatRefusesIt(t *testing.T) { ExperimentChangedWithoutItsRecord, AnswerAlreadyLandedWasRewritten, QuestionAlreadyAskedWasRewritten, + RecordAlreadyLandedWasRemoved, + ExperimentAlreadyLandedWasRenamed, } refused := make(map[string]bool) diff --git a/internal/pullrequest/read.go b/internal/pullrequest/read.go index 82ca0c4..5fdcee2 100644 --- a/internal/pullrequest/read.go +++ b/internal/pullrequest/read.go @@ -108,7 +108,10 @@ func ParseEvent(data []byte) (Event, error) { // A rename produces two paths for one entry, and both are returned. The old one // is gone at the head and the new one is not, which is what the record rule // needs: moving a file out of an experiment is a change to that experiment -// whichever end of the move a reader looks at. +// whichever end of the move a reader looks at. The new one also carries where +// it came from, because a removal beside an addition and a move are the same +// two paths otherwise, and the rule over a renamed experiment has to name both +// ends of the move. func ParseFiles(data []byte) ([]File, error) { fields := splitNul(data) var files []File @@ -131,7 +134,7 @@ func ParseFiles(data []byte) ([]File, error) { case 2: files = append(files, File{Path: fields[i+1], Gone: true}, - File{Path: fields[i+2], Gone: false}) + File{Path: fields[i+2], Gone: false, From: fields[i+1]}) } i += paths } diff --git a/internal/pullrequest/read_test.go b/internal/pullrequest/read_test.go index 9ec253c..253170b 100644 --- a/internal/pullrequest/read_test.go +++ b/internal/pullrequest/read_test.go @@ -60,7 +60,7 @@ func TestParseFiles(t *testing.T) { out: "R100\x00experiments/one/measure.go\x00experiments/two/measure.go\x00", want: []File{ {Path: "experiments/one/measure.go", Gone: true}, - {Path: "experiments/two/measure.go"}, + {Path: "experiments/two/measure.go", From: "experiments/one/measure.go"}, }, }, { diff --git a/internal/pullrequest/record.go b/internal/pullrequest/record.go index 100690f..404bc2f 100644 --- a/internal/pullrequest/record.go +++ b/internal/pullrequest/record.go @@ -76,7 +76,8 @@ type RecordChange struct { // After is the record at the head of the range, and AfterPresent says it // is still there. A record this change removes has no after. That a landed // record may not be removed at all is a different rule about a different - // failure, and it is issue #69's rather than this one's. + // failure, and it is record-already-landed-was-removed in removal.go + // rather than either of the two here. After []byte AfterPresent bool } diff --git a/internal/pullrequest/removal.go b/internal/pullrequest/removal.go new file mode 100644 index 0000000..30c4899 --- /dev/null +++ b/internal/pullrequest/removal.go @@ -0,0 +1,134 @@ +package pullrequest + +// The other half of the rule this board's central claim rests on. A record is +// not rewritten to hide what it said, which record.go holds, and it is not +// deleted, which is here. +// +// Deleting was the cheaper of the two and nothing refused it. A change that +// removes an EXPERIMENT.md, or the directory it sits in, passes every check +// that walks a tree, because the runner walks what is there and a record that +// is gone is not a record it can meet. So the rule held against editing and not +// against removing, and removing is the way to make an experiment stop having +// existed. +// +// WHY IT IS HERE AND NOT IN THE RUNNER, which is record.go's reason one step +// further on. Telling a removal from a record that was never there needs the +// version at the base of the range, which is history rather than a tree, and +// giving the runner a history reader would cost it the dependency surface +// record 0001 chose and the claim that it opens no connection leans on. This +// check already holds both ends of the range, so the comparison belongs beside +// the answer rule it completes. + +import "fmt" + +// RecordAlreadyLandedWasRemoved refuses a change that removes a record which +// was already on the branch the change lands on. +// +// The failure is ordinary rather than malicious, and that is what makes it +// likely. An experiment answered no, its prototype was removed under record +// 0004, and what is left looks like an empty directory somebody forgot to tidy. +// Tidying it takes the record with it, the tree is green afterwards, and the +// only thing that said the work happened at all is gone. +// +// What stays allowed is exactly what record 0004 already permits: the code +// goes, the record stays, and it gains the line naming the commit that removed +// the code. +const RecordAlreadyLandedWasRemoved = "record-already-landed-was-removed" + +// ExperimentAlreadyLandedWasRenamed refuses a change that moves a record which +// was already on the branch the change lands on. +// +// A slug renamed to something better months later is a removal and an addition +// as far as the tree is concerned. Every pointer at the old name stops +// resolving, including a promotion section a reader is following from another +// board, and nothing about the tree afterwards says the old name was ever used. +// +// It is refused rather than repaired, and the refusal names both paths, because +// the repair is a choice this check may not make: keep the slug, or write a new +// experiment with its own question and let the old record say where the work +// went. Renaming in silence is the only option this removes. +const ExperimentAlreadyLandedWasRenamed = "experiment-already-landed-was-renamed" + +// judgeRemovals holds every record that was on the branch at the base of the +// range to still being there at the head. +// +// THE BOUNDARIES, WRITTEN HERE RATHER THAN DISCOVERED. +// +// A record created and removed inside one branch is not covered, and it is not +// covered twice over. Such a record never reached the branch this change lands +// on, so nothing about it was made permanent, and a directory added by mistake +// and taken out again in the same pull request is ordinary work. The range is +// read from base to head, so the file is not in the diff at all, and even where +// something put it there the record would carry no version at the base. +// +// A rename is separated from a removal by what git reported, which is a +// similarity judgement rather than a fact about the change. Where the move is +// not reported as one, and a rename made together with a rewrite of the record +// is the case where it will not be, the refusal is the removal rather than the +// rename. That is red for the right reason and names the wrong repair, which is +// the residual worth knowing about before somebody meets it. +// +// WHAT THIS CANNOT DO is stop history being rewritten on the branch itself. A +// force push that removes the commit the record arrived in is refused by the +// ruleset, which refuses a non-fast-forward push and carries no bypass actors, +// and it is named here so that a green run is not read as covering it. +func judgeRemovals(change Change) Verdict { + if !change.RecordsRead { + return Verdict{Skips: []Skip{ + { + Rule: RecordAlreadyLandedWasRemoved, + Why: "this run was given no records, so nothing was read at the base of the range to be missing at the head", + }, + { + Rule: ExperimentAlreadyLandedWasRenamed, + Why: "this run was given no records, so no record was looked for at the two ends of a move", + }, + }} + } + + var verdict Verdict + if !change.FilesRead { + verdict.Skips = append(verdict.Skips, Skip{ + Rule: ExperimentAlreadyLandedWasRenamed, + Why: "this run was given no changed paths, so a record that moved cannot be told from one that went, and a move is refused as a removal", + }) + } + + moves := renames(change) + for _, record := range change.Records { + if !record.BeforePresent || record.AfterPresent { + continue + } + if to, moved := moves[record.Path]; moved { + verdict.Refusals = append(verdict.Refusals, Refusal{ + Property: ExperimentAlreadyLandedWasRenamed, + Subject: record.Path, + Detail: fmt.Sprintf("it was on the branch this lands on and this change moves it to %s, so every pointer at the old path stops resolving. Keep the slug, or leave the record where it is and let it say where the work went", to), + }) + continue + } + verdict.Refusals = append(verdict.Refusals, Refusal{ + Property: RecordAlreadyLandedWasRemoved, + Subject: record.Path, + Detail: "it was on the branch this lands on and is not at the head of this range, so an experiment that happened stops having happened. Record 0004 removes the code and keeps the record, which gains the line naming the commit that removed it", + }) + } + return verdict +} + +// renames returns where each moved path went, read out of what git reported +// rather than guessed from the content at the two ends. +// +// Guessing was the alternative and it is worse in both directions. Comparing +// the bytes of a record that went with the bytes of one that arrived calls a +// move made together with an edit a removal, and calls two unrelated records +// with the same words a move. +func renames(change Change) map[string]string { + moves := make(map[string]string) + for _, file := range change.Files { + if file.From != "" { + moves[file.From] = file.Path + } + } + return moves +} diff --git a/internal/pullrequest/removal_test.go b/internal/pullrequest/removal_test.go new file mode 100644 index 0000000..15ca671 --- /dev/null +++ b/internal/pullrequest/removal_test.go @@ -0,0 +1,214 @@ +package pullrequest + +import ( + "strings" + "testing" +) + +// The cases over a record that was on the branch a change lands on and is not +// at the head of it. 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 always the same field. Whether the record had a version at the +// base is the whole of what separates a removal this rule is about from a +// directory somebody added and took out again while working. +func removalJudgeCases() []judgeCase { + const record = "experiments/one/EXPERIMENT.md" + const moved = "experiments/reading-a-tree-of-records/EXPERIMENT.md" + landed := recordAt("answered", theLandedAnswer) + + return []judgeCase{ + { + name: "a record already on the branch that this change removes", + change: func() Change { + c := clean() + c.Files = []File{ + {Path: record, Gone: true}, + {Path: "experiments/one/measure.go", Gone: true}, + } + c.Records = []RecordChange{{ + Path: record, + Before: landed, + BeforePresent: true, + }} + return c + }(), + // The ordinary shape rather than the malicious one. The prototype + // went under record 0004, what was left looked like an empty + // directory, and tidying it took the record with it. + want: []string{RecordAlreadyLandedWasRemoved}, + }, + { + name: "an experiment directory already on the branch that this change renames", + change: func() Change { + c := clean() + c.Files = []File{ + {Path: record, Gone: true}, + {Path: moved, From: record}, + } + c.Records = []RecordChange{ + {Path: record, Before: landed, BeforePresent: true}, + {Path: moved, After: landed, AfterPresent: true}, + } + return c + }(), + // The record is still in the tree and every pointer at where it + // was has stopped resolving, including a promotion section a + // reader is following from another board. + want: []string{ExperimentAlreadyLandedWasRenamed}, + }, + { + name: "a record this change removes that was never on the branch it lands on", + change: func() Change { + c := clean() + c.Files = []File{{Path: record, Gone: true}} + c.Records = []RecordChange{{Path: record}} + return c + }(), + // The boundary the issue asks for at the check, held in the field + // the rule actually reads. Over a range read from base to head a + // record added and taken out again inside one branch is not in the + // diff at all and never reaches this rule; this case says the rule + // answers the same way where it does reach it, so the boundary does + // not rest on how the range was read. + }, + { + name: "an experiment's code removed with the record kept", + change: func() Change { + c := clean() + c.Files = []File{ + {Path: "experiments/one/measure.go", Gone: true}, + {Path: record}, + } + c.Records = []RecordChange{{ + Path: record, + Before: landed, + BeforePresent: true, + After: recordAt("answered", theLandedAnswer+"\n\n"+ + "The prototype was removed in 1111111111111111111111111111111111111111."), + AfterPresent: true, + }} + return c + }(), + // What record 0004 permits, and the case that says this rule did + // not quietly forbid it. The code goes, the record stays, and it + // gains the line naming the commit that removed it. + }, + } +} + +// TestARemovedRecordNamesWhatMayBeRemovedInstead holds the removal refusal to +// carrying the repair. Somebody meeting this has already decided the directory +// is finished with, and a refusal that only says no sends them looking for +// which of the two rules about records they have hit. +func TestARemovedRecordNamesWhatMayBeRemovedInstead(t *testing.T) { + const record = "experiments/one/EXPERIMENT.md" + + change := clean() + change.Files = []File{{Path: record, Gone: true}} + change.Records = []RecordChange{{ + Path: record, + Before: recordAt("answered", theLandedAnswer), + BeforePresent: true, + }} + + refusals := Judge(change).Refusals + if len(refusals) != 1 { + t.Fatalf("the verdict carries %d refusals, want exactly one", len(refusals)) + } + if refusals[0].Subject != record { + t.Errorf("the refusal names %q, want %q", refusals[0].Subject, record) + } + if !strings.Contains(refusals[0].Detail, "0004") { + t.Errorf("the refusal does not say what may be removed instead: %q", refusals[0].Detail) + } +} + +// TestARenameNamesBothEndsOfTheMove is the half of the issue a property +// identifier cannot carry. The repair is a choice between keeping the slug and +// writing a new experiment, and neither can be made by somebody who has been +// told only where the record used to be. +func TestARenameNamesBothEndsOfTheMove(t *testing.T) { + const from = "experiments/one/EXPERIMENT.md" + const to = "experiments/two/EXPERIMENT.md" + landed := recordAt("answered", theLandedAnswer) + + change := clean() + change.Files = []File{{Path: from, Gone: true}, {Path: to, From: from}} + change.Records = []RecordChange{ + {Path: from, Before: landed, BeforePresent: true}, + {Path: to, After: landed, AfterPresent: true}, + } + + refusals := Judge(change).Refusals + if len(refusals) != 1 { + t.Fatalf("the verdict carries %d refusals, want exactly one", len(refusals)) + } + if refusals[0].Property != ExperimentAlreadyLandedWasRenamed { + t.Fatalf("the verdict refuses %s, want %s", refusals[0].Property, ExperimentAlreadyLandedWasRenamed) + } + if refusals[0].Subject != from { + t.Errorf("the refusal names %q as its subject, want the path the record was at, %q", refusals[0].Subject, from) + } + if !strings.Contains(refusals[0].Detail, to) { + t.Errorf("the refusal never names where the record went: %q", refusals[0].Detail) + } +} + +// TestARenamedRecordSurvivesTheJoinFromTheDiff is the join between the two +// halves of this package, and it was added because deleting the pairing in the +// parser left every case above green. Each half is proved on its own: the cases +// hand the rule a value somebody typed, and the parser is held to what git +// prints. Neither of them notices the pairing being dropped on the way from one +// to the other, which is one field in one branch of ParseFiles and exactly the +// line somebody tidying it would take out. +// +// The diff below is what git prints for a renamed experiment, in the shape +// TestParseFiles already carries. +func TestARenamedRecordSurvivesTheJoinFromTheDiff(t *testing.T) { + const from = "experiments/one/EXPERIMENT.md" + const to = "experiments/two/EXPERIMENT.md" + landed := recordAt("answered", theLandedAnswer) + + files, err := ParseFiles([]byte("R100\x00" + from + "\x00" + to + "\x00")) + if err != nil { + t.Fatalf("cannot read the diff: %v", err) + } + + change := clean() + change.Files = files + change.Records = []RecordChange{ + {Path: from, Before: landed, BeforePresent: true}, + {Path: to, After: landed, AfterPresent: true}, + } + + properties := Judge(change).Properties() + if len(properties) != 1 || properties[0] != ExperimentAlreadyLandedWasRenamed { + t.Fatalf("the verdict is %v, want exactly %s", properties, ExperimentAlreadyLandedWasRenamed) + } +} + +// TestAMoveNothingReportedAsOneIsRefusedAsARemoval holds the residual the rule +// declares. Where git did not call the move a rename, the same change is +// refused for the same reason under the other property, and the repair the +// message names is the wrong one of the two. It is written down here so that a +// reader meeting it knows it was chosen rather than missed. +func TestAMoveNothingReportedAsOneIsRefusedAsARemoval(t *testing.T) { + const from = "experiments/one/EXPERIMENT.md" + const to = "experiments/two/EXPERIMENT.md" + landed := recordAt("answered", theLandedAnswer) + + change := clean() + change.Files = []File{{Path: from, Gone: true}, {Path: to}} + change.Records = []RecordChange{ + {Path: from, Before: landed, BeforePresent: true}, + {Path: to, After: landed, AfterPresent: true}, + } + + properties := Judge(change).Properties() + if len(properties) != 1 || properties[0] != RecordAlreadyLandedWasRemoved { + t.Fatalf("the verdict is %v, want exactly %s", properties, RecordAlreadyLandedWasRemoved) + } +}