From 10550f1ac9b348b6e43fccff5cc2b44ca3f76098 Mon Sep 17 00:00:00 2001 From: Ezekiel Lopez Date: Tue, 18 Aug 2026 17:39:29 -0700 Subject: [PATCH] feat: retain approvals across pure renames Skips a hunk whose whole difference is one identifier substituted everywhere it appeared: -def handler(request): -RETRY_LIMTI = 3 +def handler(req): +RETRY_LIMIT = 3 Deliberately strict: the two sides must hold the same tokens in the same order and the same number, exactly one name may have changed, and the old name may not survive anywhere, so a rename carrying anything else along with it dismisses as before. Keywords, numbers and the text inside string literals are not names and never take part in a substitution: swapping and for or, or True for False, wears the shape of a one-word rename while changing behaviour. A consistent substitution is not on its own a rename. The same shape spells a call swapped for a different function, an enum member swapped for another, or an error type swapped at a raise. So the name also has to be one the change plainly owns - spelled out where it is used, by code the hunk itself holds. A substitution is refused wherever the name appears in call position, in a decorator, as a member the hunk does not itself assign, as the operand of a raise, or as the name a class, def or func declares. What is left is the local shape: a local, a parameter, a keyword argument, a comprehension variable, a constant, plus a test renamed to another test name, which a runner still finds by its prefix. Like comments, this needs to know the language: the rule reads punctuation to decide what a name is being used for, and what punctuation means is a fact about a language. A file we cannot name keeps its renames. Opt-in only, and never enabled by the umbrella. The behaviour is unchanged, which is the argument for the flag; the blast radius is not, which is the argument for it having its own. A rename can be a public API change, and if you own only the defining file you would never re-read the callers. --- README.md | 8 +- internal/git/normalize.go | 20 +- internal/git/normalize_renames.go | 168 ++++++++++++++++ internal/git/normalize_renames_test.go | 255 +++++++++++++++++++++++++ 4 files changed, 447 insertions(+), 4 deletions(-) create mode 100644 internal/git/normalize_renames.go create mode 100644 internal/git/normalize_renames_test.go diff --git a/README.md b/README.md index 59a7701..bd41603 100644 --- a/README.md +++ b/README.md @@ -370,10 +370,16 @@ A change is checked one hunk at a time. Everything the hunk adds is compared aga `comments` ignores lines which are nothing but a comment, and comments appended to the end of a line which is otherwise code, so an annotation added above or beside code the approver already read is not a change to it. Which markers open a comment is decided by the file's extension, because the same characters spell code elsewhere: `//` divides in Python, `--` decrements in C, and neither opens a comment in CSS. A file whose extension names no language we know is left with no markers at all rather than a guess, so its comments are reviewed like any other change. Markers which also spell something else within the language which uses them - `#` opening a preprocessor directive or an id selector, `--` spelling a command flag, `*` spelling a dereference or a splat - only count as comments where they stand on their own. A line which quotes anything keeps whatever it ends with: a marker inside a string literal is text rather than a comment, and telling the two apart takes a parser for the language the line is written in. -Directives are deliberately excluded. A comment addressed to a tool rather than to the next reader - `noqa`, `nosec`, `# type: ignore`, `eslint-disable`, `@ts-expect-error`, `istanbul ignore`, `//go:build`, `//nolint`, `NOSONAR`, `pragma: no cover` and the rest of that family - is configuration wearing a comment's syntax. Adding one silences a checker, changing its argument widens what the checker will accept, and removing one turns a check back on, all while leaving the runtime code byte for byte identical. That is exactly the change an approver would want in front of them, so a comment which opens with a known directive is compared like the code it sits in and a change to it dismisses the approval. The match is anchored at the start of the comment and requires the directive word to end where the directive does, so prose which merely opens with a similar word - "pragmatic for now", "eslintrc covers this rule" - is a comment like any other. +Directives are deliberately excluded. A comment addressed to a tool rather than to the next reader - `noqa`, `nosec`, `# type: ignore`, `eslint-disable`, `@ts-expect-error`, `istanbul ignore`, `//go:build`, `//nolint`, `NOSONAR`, `pragma: no cover` and the rest of that family - is configuration wearing a comment's syntax. Adding one silences a checker, changing its argument widens what the checker will accept, and removing one turns a check back on, all while leaving the runtime code byte for byte identical. That is exactly the change an approver would want in front of them, so a comment which opens with a known directive is compared like the code it sits in and a change to it dismisses the approval. The same goes for a rename: a word inside a directive names a rule, an error code or a build tag rather than something the code calls by name, so `renames` will not read a substitution out of a hunk which touches one. The match is anchored at the start of the comment and requires the directive word to end where the directive does, so prose which merely opens with a similar word - "pragmatic for now", "eslintrc covers this rule" - is a comment like any other. `formatting` ignores everything `whitespace` ignores and, on top of that, braces, semicolons and trailing commas. Parentheses are always significant, since their placement decides operator precedence and separates a call from a reference. Only the shape of the code is compared, so a change which moves a statement into or out of a block without editing the statement itself - re-indenting a line in an indentation-sensitive language, or dropping the braces around a multi-statement body - reads as formatting. A pair of braces which closes over nothing is an exception: an empty argument, body or literal says something the code does not say without it, so adding or removing one is a change like any other. +`renames` ignores one identifier substituted for another everywhere it appeared, and reads whatever the other enabled flags leave behind. It is deliberately strict: the two sides must hold the same tokens in the same order and the same number, only one name may have changed, and the old name may not survive anywhere, so a rename carrying any other change along with it dismisses as before. Keywords, numbers and the text inside string literals are not names and never take part in a substitution. + +Like `comments`, `renames` needs to know what language the file is written in. The rule reads punctuation to decide what a name is being used for - the parentheses of a call, the `@` of a decorator, the `.` of an access - and what those spell is a fact about a language, so a file whose name places it in no language we know keeps its renames and is reviewed like any other change. + +A consistent substitution is not on its own a rename - the same shape spells a call swapped for a different function, an enum member swapped for another member of the same enum, or an error type swapped at a `raise`. So the identifier also has to be one the change plainly owns: something spelled out where it is used, by code the hunk itself holds. A substitution is refused wherever the name appears in call position, in a decorator, as an attribute or member the hunk does not itself assign, as the operand of a `raise` or `throw`, or as the name a `class`, `def` or `func` declares. What is left is the local shape of the rule - a local, a parameter, a keyword argument, a comprehension variable, a constant - plus a test renamed to another test name, which whatever runs it still finds by its prefix. Even so, this flag stays off unless it is asked for: a name can be reached from outside the hunk in ways a diff cannot see. + #### Require Both Branch Reviewers (Ownership Handoffs) The `require_both_branch_reviewers` feature enables self-service ownership transfers by requiring approval from codeowners defined in **BOTH** the base branch and the PR branch. This creates an AND relationship between ownership rules from both branches. diff --git a/internal/git/normalize.go b/internal/git/normalize.go index dd5ba59..c12070a 100644 --- a/internal/git/normalize.go +++ b/internal/git/normalize.go @@ -12,7 +12,8 @@ import ( // normalizer reports whether a hunk's two sides are the same code once the enabled // flags' noise is stripped. Renames is not a step: it compares what they leave. type normalizer struct { - steps []normalizeStep + steps []normalizeStep + renames bool } // normalizeStep rewrites one side of a hunk, its lines joined by newlines. The @@ -34,20 +35,33 @@ func newNormalizer(retention *owners.ApprovalRetention) normalizer { if retention.FormattingEnabled() { n.steps = append(n.steps, inAnyLanguage(collapseFormatting)) } + n.renames = retention.RenamesEnabled() return n } // A flag may only retain a hunk it actually read, so anything the enabled steps // cannot account for stays non-trivial and an approval is kept only on purpose. func (n normalizer) isTrivial(fileName string, hunk *diff.Hunk) bool { - if len(n.steps) == 0 { + if len(n.steps) == 0 && !n.renames { return false } added, removed, ok := hunkBlocks(hunk.Body) if !ok { return false } - return n.normalize(fileName, added) == n.normalize(fileName, removed) + normalizedAdded, normalizedRemoved := n.normalize(fileName, added), n.normalize(fileName, removed) + // Renames needs a substitution to point at, so it never carries a hunk alone. + if len(n.steps) > 0 && normalizedAdded == normalizedRemoved { + return true + } + // Renames reads punctuation - a call's parens, a decorator's `@`, an access's + // `.` - and what that means is a fact about a language, so refuse unknown files. + if !n.renames || !knownLanguage(fileName) || !isPureRename(normalizedAdded, normalizedRemoved) { + return false + } + // A word inside a directive names a rule or an error code, read by a tool, so + // substituting one changes what is enforced rather than what anything is called. + return !holdsDirective(fileName, added) && !holdsDirective(fileName, removed) } func (n normalizer) normalize(fileName, block string) string { diff --git a/internal/git/normalize_renames.go b/internal/git/normalize_renames.go new file mode 100644 index 0000000..b0f08cd --- /dev/null +++ b/internal/git/normalize_renames.go @@ -0,0 +1,168 @@ +package git + +import ( + "strings" +) + +// Swapping `and` for `or` wears the shape of a one-word rename while changing +// behavior, so no keyword may take part in a substitution. +var keywords = wordSet(` + and or not in is if elif else elseif unless for foreach while do switch case + default break continue return yield await async try catch except finally + throw raise new delete typeof instanceof void null nil none true false this + self super var let const def fn func function class struct interface enum + trait impl import export include from as with using namespace package module + pass lambda global nonlocal assert del defer go select chan range map static + public private protected final abstract extends implements override readonly + goto sizeof begin end then`) + +func wordSet(words string) map[string]bool { + set := make(map[string]bool) + for _, word := range strings.Fields(words) { + set[word] = true + } + return set +} + +// A name is the only kind a rename may move; everything else spells structure, +// a value, or text which is read rather than called. +type token struct { + text string + isName bool +} + +// Deliberately strict: same tokens in the same order and number, exactly one name +// changed, the old name surviving nowhere, and the name one the change owns. +func isPureRename(added, removed string) bool { + was, now := tokenize(removed), tokenize(added) + if len(was) != len(now) { + return false + } + + var from, to token + var at []int + for i := range was { + if was[i] == now[i] { + continue + } + if !was[i].isName || !now[i].isName { + return false + } + if len(at) > 0 && (was[i] != from || now[i] != to) { + return false + } + from, to = was[i], now[i] + at = append(at, i) + } + if len(at) == 0 { + return false + } + // An occurrence the change left standing rules the rename out. + for i := range was { + if was[i] == from && now[i] == from { + return false + } + } + return ownsName(now, at, from, to) +} + +// What catches an error names the same type, from a handler somewhere else. +var raiseOperands = wordSet("raise throw") + +// A definition is referred to everywhere except the hunk declaring it. +var declarationKeywords = wordSet(` + class def func function fn struct interface enum trait`) + +// Owned only where the hunk holds the code that uses it. Elsewhere the other half +// is out of sight, and the substitution repoints the code or points it at nothing. +func ownsName(tokens []token, at []int, from, to token) bool { + // A member the hunk assigns is one it brings into being. + defined := definesMember(tokens, at) + + for _, i := range at { + previous, next := tokenAt(tokens, i-1), tokenAt(tokens, i+1) + // Read before a call: a definition spells its name in front of the same + // parentheses a call would. + switch { + case previous.text == "@": + return false + case raiseOperands[strings.ToLower(previous.text)]: + return false + case declarationKeywords[strings.ToLower(previous.text)]: + // A runner finds a test by its prefix, not its full name, so a test + // renamed to another test name is still run the same way. + if !namesATest(from.text) || !namesATest(to.text) { + return false + } + case next.text == "(": + return false + case previous.text == ".": + if !defined { + return false + } + } + } + return true +} + +// Only a plain assignment defines: an augmented one writes its operator before the +// `=`, and a comparison doubles it, so neither is a single `=` following the name. +func definesMember(tokens []token, at []int) bool { + for _, i := range at { + if tokenAt(tokens, i+1).text != "=" { + continue + } + if tokenAt(tokens, i+2).text == "=" { + continue + } + return true + } + return false +} + +const testNamePrefix = "test" + +func namesATest(name string) bool { + return strings.HasPrefix(strings.ToLower(name), testNamePrefix) +} + +// An index off either end returns an empty token, so a hunk edge matches no rule. +func tokenAt(tokens []token, i int) token { + if i < 0 || i >= len(tokens) { + return token{} + } + return tokens[i] +} + +func tokenize(block string) []token { + var tokens []token + for i := skipSpace(block, 0); i < len(block); i = skipSpace(block, i) { + if _, _, end, ok := stringLiteralAt(block, i); ok { + // A literal is opaque: a word changed inside it is not a rename. + tokens = append(tokens, token{text: block[i:end]}) + i = end + continue + } + if end := identifierEnd(block, i); end > i { + tokens = append(tokens, token{text: block[i:end], isName: isName(block[i:end])}) + i = end + continue + } + tokens = append(tokens, token{text: block[i : i+1]}) + i++ + } + return tokens +} + +func identifierEnd(block string, i int) int { + end := i + for end < len(block) && isIdentifierByte(block[end]) { + end++ + } + return end +} + +// A run opening with a digit is a value spelled out, not a name for one. +func isName(text string) bool { + return !(text[0] >= '0' && text[0] <= '9') && !keywords[strings.ToLower(text)] +} diff --git a/internal/git/normalize_renames_test.go b/internal/git/normalize_renames_test.go new file mode 100644 index 0000000..55f82c8 --- /dev/null +++ b/internal/git/normalize_renames_test.go @@ -0,0 +1,255 @@ +package git + +import ( + "testing" + + owners "github.com/multimediallc/codeowners-plus/internal/config" +) + +// One name substituted everywhere it appeared and nothing else. Each names +// something spelled out where it is used, so the hunk holds the whole change. +var renameOnlyHunks = []hunkCase{ + { + name: "parameter renamed", + file: "views.py", + body: `-def handler(request): ++def handler(req):`, + }, + { + name: "loop variable renamed at every use", + file: "billing.py", + body: `- for item in items: +- total += item.price ++ for entry in items: ++ total += entry.price`, + }, + { + name: "unused binding renamed", + file: "views.py", + body: `- handler = lambda user: None ++ handler = lambda _: None`, + }, + { + name: "local renamed at every use", + file: "billing.py", + body: `- total = price * quantity +- return total ++ subtotal = price * quantity ++ return subtotal`, + }, + { + name: "keyword parameter renamed", + file: "cache.py", + body: `-def connect(host, retry_count=3): ++def connect(host, retries=3):`, + }, + { + name: "comprehension variable renamed", + file: "billing.py", + body: `- names = [row.label for row in rows] ++ names = [entry.label for entry in rows]`, + }, + { + name: "constant typo fixed at every use", + file: "cache.py", + body: `-RETRY_LIMTI = 3 +- if count > RETRY_LIMTI: ++RETRY_LIMIT = 3 ++ if count > RETRY_LIMIT:`, + }, + { + name: "test renamed to another test name", + file: "test_billing.py", + body: `-def test_totals_add_up(self): ++def test_totals_include_tax(self):`, + }, + { + name: "member the hunk itself assigns renamed", + file: "cache.py", + body: `- self.retry_count = 0 +- return self.retry_count ++ self.retries = 0 ++ return self.retries`, + }, +} + +// Each wears the shape of a substitution while the other half of what it renames +// - a call's body, a framework's hook, an error handler - lives out of sight. +var renamedNameNotOwnedHunks = []hunkCase{ + { + name: "predicate swapped at its call site", + file: "billing.py", + body: `- if is_active(account): ++ if can_post(account):`, + }, + { + name: "helper renamed at its call site", + file: "widget.js", + body: `- result = computeTotal(items) ++ result = calculateTotal(items)`, + }, + { + name: "decorator swapped", + file: "test_views.py", + body: `-@override_settings ++@modify_settings`, + }, + { + name: "boolean property swapped in a predicate", + file: "widget.js", + body: `- if (account.isActive) { ++ if (account.canPost) {`, + }, + { + name: "enum member swapped", + file: "billing.py", + body: `- state = Status.ACTIVE ++ state = Status.PENDING`, + }, + { + name: "raised error type swapped", + file: "billing.py", + body: `- raise ValidationError ++ raise ConfigError`, + }, + { + name: "thrown error type swapped", + file: "widget.js", + body: `- throw ValidationError ++ throw ConfigError`, + }, + { + name: "overridden hook renamed away from the name it is dispatched to", + file: "views.py", + body: `-def get_queryset(self): ++def build_queryset(self):`, + }, + { + name: "class renamed", + file: "views.py", + body: `-class OrderView: ++class OrderPage:`, + }, + { + name: "exported function renamed", + file: "widget.js", + body: `-function computeTotal(items) { ++function calculateTotal(items) {`, + }, +} + +// The rule reads punctuation to decide how a name is used, and what punctuation +// means is a fact about a language, so an unnamed file's guards do not apply. +func TestRenamesNeedAKnownLanguage(t *testing.T) { + retentions := map[string]*owners.ApprovalRetention{ + "renames alone": steps(renamesOn), + "every step": steps(renamesOn, commentsOn, whitespaceOn, formattingOn, stringLiteralsOn), + } + + for name, retention := range retentions { + t.Run(name, func(t *testing.T) { + n := newNormalizer(retention) + assertTrivial(t, n, renameOnlyHunks) + + for _, file := range []string{ + "data.unknownext", + "CHANGELOG", + "", + "vendor/blob", + } { + t.Run(file, func(t *testing.T) { + assertSignificant(t, n, withFile(file, renameOnlyHunks)) + }) + } + }) + } +} + +func TestRenamesRetainsRenameOnlyHunks(t *testing.T) { + retentions := map[string]*owners.ApprovalRetention{ + "renames alone": steps(renamesOn), + "every step": steps(renamesOn, commentsOn, whitespaceOn, formattingOn, stringLiteralsOn), + } + + for name, retention := range retentions { + t.Run(name, func(t *testing.T) { + assertTrivial(t, newNormalizer(retention), renameOnlyHunks) + }) + } +} + +func TestRenamesAloneDismissesEverythingElse(t *testing.T) { + n := newNormalizer(steps(renamesOn)) + + assertSignificant(t, n, significantHunks) +} + +// With only renames enabled there is no substitution to point at, so a hunk which +// normalizes to nothing dismisses rather than riding in on a flag that never read it. +func TestRenamesAloneDismissesAnEmptiedHunk(t *testing.T) { + if newNormalizer(steps(renamesOn)).isTrivial("billing.py", hunkOf("+")) { + t.Error("expected an added blank line to be non-trivial with only renames enabled") + } +} + +// Each substitutes one identifier consistently and would read as a rename on that +// alone, while what answers to the old name lives outside the hunk. +func TestRenamesDismissANameTheChangeDoesNotOwn(t *testing.T) { + retentions := map[string]*owners.ApprovalRetention{ + "renames alone": steps(renamesOn), + "every step": steps(renamesOn, commentsOn, whitespaceOn, formattingOn, stringLiteralsOn), + } + + for name, retention := range retentions { + t.Run(name, func(t *testing.T) { + assertSignificant(t, newNormalizer(retention), renamedNameNotOwnedHunks) + }) + } +} + +// The umbrella never turns renames on by itself, so a rename dismisses until +// the flag is set to true in as many words. +func TestRenamesStayOffUntilAskedFor(t *testing.T) { + retentions := map[string]*owners.ApprovalRetention{ + "umbrella alone": {Enabled: true}, + "every other step on": steps(commentsOn, whitespaceOn, formattingOn, stringLiteralsOn), + "opted in but disabled": {Enabled: true, Renames: flag(false)}, + } + + for name, retention := range retentions { + t.Run(name, func(t *testing.T) { + assertSignificant(t, newNormalizer(retention), renameOnlyHunks) + }) + } +} + +// Two names changing is not one rename. Each substitution on its own would +// read as one, so the rule has to hold the pair it found and refuse a second. +var twoNamesChangedHunks = []hunkCase{ + { + name: "two locals renamed in one hunk", + file: "billing.py", + body: `- total = price * quantity ++ subtotal = cost * quantity`, + }, + { + name: "parameter and its use renamed to different names", + file: "views.py", + body: `-def handler(request, session): +- return request.user, session ++def handler(req, ctx): ++ return req.user, ctx`, + }, + { + name: "one rename plus one value changed", + file: "cache.py", + body: `- retry_count = 3 +- return retry_count ++ retries = 5 ++ return retries`, + }, +} + +func TestRenamesDismissTwoNamesChanging(t *testing.T) { + assertSignificant(t, newNormalizer(steps(renamesOn)), twoNamesChangedHunks) +}