From a0a973760f25a1c6105ddb0d42428ea2d70dbd55 Mon Sep 17 00:00:00 2001 From: jordonpeterson Date: Mon, 3 Aug 2026 13:19:15 -0600 Subject: [PATCH] fix(plan): decide amend-vs-narrow by pattern containment, not tree snapshot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The planner chose to amend a rule in place whenever every TRACKED file that rule won was inside the operation's scope. That is a property of the current commit, but a CODEOWNERS rule governs files that do not exist yet. Reported case: with `path/app/build.gradle` the only tracked file under `path/app/`, `add_owner(*.gradle, @team)` amended `path/app/* @b` in place, handing @team every future non-gradle file in that directory. The same snapshot reasoning was in remove_owner, and in set_owners via matchSetEquals, where it silently transferred an entire directory's ownership away. Replaces the check with pattern.Contains — sound containment of the pattern LANGUAGES. Patterns are tokenized into segment sequences using the same normalization the matcher compiles (extracted as normalizeSegs so the two cannot drift), then walked for containment. Contains is deliberately incomplete: false means "unproven", never "disjoint". Two subtleties the brute-force soundness test caught: - A trailing slash does NOT anchor a single-segment pattern. `docs/` gets an implicit leading `**/`, so it matches `web/docs/spec.md` — a textual prefix comparison read it as root-anchored and amended it for the scope `/docs/`. - A final `*` compiles through a different arm than a final glob, so `/x/*` matches exactly one level while `/x/foo` also matches everything beneath it. Derived narrowing patterns are now proven rather than trusted: a candidate must stay inside both the scope and the rule, and match exactly the right tracked paths. That turns the junk derivations for directory-name scopes (`x`, `docs`), which removePass re-inserted every pass before reporting "did not converge", into ordinary amends. CODEOWNERS cannot express `dir/*` ∩ `*.gradle` — there is no way to say "one level AND matching the glob", since `dir/*.gradle` also matches under a DIRECTORY named `.gradle`. Refusing would make add_owner(*.gradle, ...) impossible on any repo with a `dir/*` rule, so the narrowing rule is emitted with a warning disclosing the residual. It can never leak the new owner out of scope or withhold it; only a co-owner can be wrong. Where the rule's reach is expressible (`/svc` -> `svc/**/*.gradle`) the derivation is proven and silent. Existing tests are unchanged; all additions. Verified with `make all`, the 500k-case differential fuzz against the hmarr oracle (0 mismatches), and plan/apply/plan idempotence across the new derivation shapes. Co-Authored-By: Claude Opus 5 --- docs/BEHAVIOR.md | 104 ++++++++++- internal/pattern/contains.go | 264 ++++++++++++++++++++++++++++ internal/pattern/contains_test.go | 152 +++++++++++++++++ internal/pattern/pattern.go | 34 ++-- internal/plan/plan.go | 164 +++++++++++++----- internal/plan/plan_test.go | 275 ++++++++++++++++++++++++++++++ 6 files changed, 931 insertions(+), 62 deletions(-) create mode 100644 internal/pattern/contains.go create mode 100644 internal/pattern/contains_test.go diff --git a/docs/BEHAVIOR.md b/docs/BEHAVIOR.md index 4ed6cc3..80515d8 100644 --- a/docs/BEHAVIOR.md +++ b/docs/BEHAVIOR.md @@ -464,15 +464,39 @@ compile error surfaced to the caller, never a silent negation. SPEC S-6: paths are case-sensitive. `/Src/` on a tree containing `src/` is a dead rule that looks fine on a case-insensitive filesystem. +### `TestContainsIsReflexive` + +Reflexivity: a pattern always contains itself, whatever its shape. + +### `TestContainsIsSound` + +Contains must be SOUND: it may answer false when containment holds, but it +must never answer true when a concrete path matches `inner` and not `outer`. +An unsound answer here lets the planner amend a rule in place and silently +hand an owner every future file that rule will ever match — the defect this +whole mechanism exists to prevent. + +### `TestContainsKnownPairs` + +The containments the planner relies on to amend a rule in place, and the +near-miss shapes it must NOT accept. + +### `TestContainsRejectsInvalid` + +Contains must not blow up or report true on patterns Compile rejects. + ## internal/plan -> White-box test for the remove_owner settling logic (second-review -> regression): divergence between the pure transform desired∖{owner} and the -> file's actual resolution is accepted ONLY under --on-empty=inherit, where -> rule deletion legitimately resurrects owners from surviving rules. Under -> any other policy no rule is deleted, so divergence means a synthesis bug -> (or a bad earlier batched edit) and must REFUSE — accepting it would -> launder the error past the gate. +> Package plan_test encodes the spec's acceptance tests for Engine A. +> +> The planner takes intent-level ops, computes the resolved ownership +> before/after over the real tree, synthesizes line edits, and GATES the +> result on two invariants: +> +> INV-1: every path in scope resolves to exactly what the op requires. +> INV-2: every path outside scope resolves to exactly what it did before. +> +> A plan that cannot be proven is refused (exit 2), never guessed at. ### `TestAddOwner_CoversUnownedPaths` @@ -506,6 +530,12 @@ TestShape4_TreeConfirmationRefusesInexactDerivation. INV-5 property: parse→serialize round-trips any generated file (plus junk mutations) byte-identically. +### `TestR2_ContainmentIsSemanticNotTextual` + +Containment must be semantic, not textual. Each of these rules is genuinely +inside its op's scope, so amending in place is sound and the planner must not +refuse or bloat the file. A textual last-segment comparison rejected them all. + ### `TestR2_FileGlobScopeAcrossAnchoredRules` R-2, shape 4: a FILE-GLOB scope crossing anchored directory rules. @@ -528,6 +558,31 @@ Shape 4 with the rule written as an UNANCHORED directory (`app2/`, no leading slash). Such a rule matches app2/ at any depth, so the narrowing must too: `**/app2/**/*.gradle`, not `/app2/**/*.gradle`. +### `TestR2_FileGlobScopeDoesNotNarrowDirectoryRuleOnRemove` + +remove_owner shares the amend decision, so it shared the bug: removing +@b for *.gradle must not strip @b from the whole directory. + +### `TestR2_FileGlobScopeDoesNotWidenAnchoredDirRule` + +Same snapshot trap for the other common directory shape: an anchored rule +with no trailing slash, whose match set is a whole subtree. + +### `TestR2_FileGlobScopeDoesNotWidenDirectoryRule` + +A file-glob scope must never widen a directory rule. The amend-in-place +decision used to be a SNAPSHOT property ("every tracked file this rule wins +is in scope"), but a CODEOWNERS rule governs files that do not exist yet. +With build.gradle the only tracked file under path/app/, the planner amended +`path/app/* @b` to `path/app/* @b @gradle` — handing @gradle every future +non-gradle file in that directory. It must insert a narrowing +`path/app/*.gradle` rule instead. (Reported by a user.) + +### `TestR2_FileGlobScopeDoesNotWidenDirectoryRuleOnSet` + +The same, in the shape the user reported: a file-glob scope must not rewrite +a directory rule's owner set wholesale. + ### `TestR2_FileGlobScopeRemoveOwnerAcrossAnchoredRule` The same derivation serves remove_owner, which shares intersectPattern: @@ -546,6 +601,14 @@ it represents. Every .gradle file added anywhere later silently inherits /app2's owner. When an anchored derivation exists it is the tighter of two equally-exact patterns and must be preferred. +### `TestR2_InexactNarrowingIsDisclosed` + +Review finding: a `dir/*` rule governs exactly ONE level, but no CODEOWNERS +pattern says "one level AND matching *.gradle" — `path/app/*.gradle` also +matches files under a DIRECTORY named `.gradle`. The narrowing rule is exact +for every tracked file, so it is emitted, but the residual must be disclosed +rather than silently presented as proven. + ### `TestR2_ScopeNestedInsideBroaderAnchoredRule` A scope nested INSIDE a broader anchored rule: the broad rule governs @@ -560,11 +623,36 @@ add_owner over a scope that intersects an UNANCHORED broad rule whose match set extends outside the scope: the planner must synthesize an intersection pattern, and the out-of-scope paths governed by that rule stay untouched. +### `TestR2_UnanchoredDirectoryRuleIsNotAmendedForAnchoredScope` + +Review finding: a trailing slash does NOT anchor a single-segment pattern. +`docs/` is one segment, so gitignore gives it an implicit leading `**/` and +it matches `web/docs/spec.md`. A textual prefix comparison read it as +root-anchored and amended it in place for the scope `/docs/`, handing @x +every `docs` directory in the repo — the reported bug in a shape the first +fix did not cover. + ### `TestR4_AmendPreferredOverInsert` SPEC R-4: prefer amending an existing exact-match line over inserting. Without this, repeated automated runs grow the file without bound. +### `TestR4_SetOwnersDoesNotAmendABroaderRule` + +set_owners shared the snapshot bug: it amended the last intersecting rule +whenever that rule's TRACKED match set equalled the scope set. With +svc/sub/a.go the only tracked file under /svc/, it rewrote `/svc/ @b` to +`/svc/ @g` — silently transferring the entire /svc/ tree away from @b. + +### `TestR6_DirectoryNameScopeConverges` + +Review finding: a single-segment scope that is a plain directory NAME (`x`, +`docs`) denotes a whole subtree, not a filename glob. Deriving a narrowing +pattern as though it were a glob produced junk (`x/**/x`) that matched none +of the group's paths, so removePass re-inserted it every pass and then +reported "did not converge — file structure defeats the writer" for a +two-line file. The rule is inside the scope, so it must simply be amended. + ### `TestR6_EmptyPolicies` SPEC R-6: remove_owner emptying an owner set requires an explicit policy. @@ -864,4 +952,4 @@ DIFFERENT states; transitioning between them is a real ownership change. --- -131 documented test cases across 11 packages. +144 documented test cases across 11 packages. diff --git a/internal/pattern/contains.go b/internal/pattern/contains.go new file mode 100644 index 0000000..b4fba08 --- /dev/null +++ b/internal/pattern/contains.go @@ -0,0 +1,264 @@ +package pattern + +import ( + "regexp" + "strings" +) + +// Contains reports whether every path matching `inner` also matches `outer` — +// containment of the pattern LANGUAGES, not of their match sets over some +// snapshot of a file tree. Callers use it to decide whether editing a rule in +// place can affect anything outside an operation's scope, and a CODEOWNERS rule +// governs files that do not exist yet, so a tree-based answer is an accident of +// what happens to be committed today. +// +// Contains is SOUND but deliberately incomplete: it never reports true for a +// pair where containment fails, and answers false whenever it cannot prove the +// containment. Callers must treat false as "unknown", not as "disjoint". +// +// Paths are repo-relative FILE paths (see Match), so a pattern ending in `/**` +// is taken to require at least one further segment. +func Contains(outer, inner string) bool { + if outer == inner { + return true + } + // "/" matches nothing: the empty language is contained in everything, and + // contains only itself. + if inner == "/" { + return true + } + if outer == "/" { + return false + } + ot, ok := tokenize(outer) + if !ok { + return false + } + it, ok := tokenize(inner) + if !ok { + return false + } + if universal(ot) { + return true + } + memo := make(map[[2]int]bool, len(ot)*len(it)) + return covers(ot, it, 0, 0, memo) +} + +// universal reports whether a token sequence matches every non-empty path. +// "*" and "**" both normalize to a run of tokMany around a single tokAny1, +// which the segment-by-segment walk cannot recognize on its own: the walk +// pairs tokens positionally, so it cannot see that one tokAny1 plus a tokMany +// already covers "any path with at least one segment". +func universal(toks []token) bool { + ones, manys := 0, 0 + for _, t := range toks { + switch t.kind { + case tokSeg: + return false + case tokAny1: + ones++ + default: + manys++ + } + } + return ones == 1 && manys > 0 +} + +// allMany reports whether every token is a tokMany, so the run matches any +// number of segments including none. +func allMany(toks []token) bool { + for _, t := range toks { + if t.kind != tokMany { + return false + } + } + return true +} + +// producesSegment reports whether a token run must yield at least one segment. +func producesSegment(toks []token) bool { + for _, t := range toks { + if t.kind != tokMany { + return true + } + } + return false +} + +// Token kinds. A pattern is modeled as a sequence of tokens over path segments. +const ( + tokSeg = iota // exactly one segment, matching a single-segment glob + tokAny1 // exactly one segment, unconstrained ("*") + tokMany // zero or more segments ("**", or an implicit descendant tail) +) + +type token struct { + kind int + glob string // tokSeg only +} + +// tokenize converts a pattern into its token sequence, mirroring the regex +// buildPatternRegex emits for each normalized segment. +func tokenize(pat string) ([]token, bool) { + if pat == "" || pat[0] == '!' || strings.Contains(pat, "***") { + return nil, false + } + segs := normalizeSegs(pat) + last := len(segs) - 1 + var out []token + for i, seg := range segs { + switch seg { + case "**": + // A leading or middle "**" compiles to an optional run of + // segments. A TRAILING "**" compiles to "/.*", which requires the + // separator — for file paths that means at least one more segment. + if i == last { + out = append(out, token{kind: tokAny1}) + } + out = append(out, token{kind: tokMany}) + case "*": + out = append(out, token{kind: tokAny1}) + default: + out = append(out, token{kind: tokSeg, glob: seg}) + } + } + // A final literal-or-glob segment also matches descendants ("(?:/.*)?"). + // A final bare "*" does NOT: buildPatternRegex emits it from the `case "*"` + // arm, which never appends that tail — so "/x/*" matches exactly one level + // while "/x/foo" also matches everything beneath "x/foo". + if segs[last] != "**" && segs[last] != "*" { + out = append(out, token{kind: tokMany}) + } + return out, true +} + +// covers reports whether outer[oi:] matches every path outer[ii:] can produce. +func covers(outer, inner []token, oi, ii int, memo map[[2]int]bool) bool { + key := [2]int{oi, ii} + if v, seen := memo[key]; seen { + return v + } + memo[key] = false // conservative on cycles + + r := func() bool { + if oi == len(outer) { + // Outer is spent, so it can absorb nothing further. Any leftover + // inner token — including a tokMany, which is nullable but need not + // be null — can produce a segment outer cannot cover. + return ii == len(inner) + } + // A universal remainder covers whatever is left without pairing tokens + // off one by one. "/x/" ends in [tokAny1, tokMany] — "one or more + // segments, anything" — which the positional walk alone cannot match + // against an inner "**" that stands for a variable number of segments. + if allMany(outer[oi:]) { + return true + } + if universal(outer[oi:]) && producesSegment(inner[ii:]) { + return true + } + ot := outer[oi] + if ot.kind == tokMany { + // Absorb zero inner segments, or swallow one more inner token. + if covers(outer, inner, oi+1, ii, memo) { + return true + } + return ii < len(inner) && covers(outer, inner, oi, ii+1, memo) + } + // Outer needs exactly one segment here, so inner must supply exactly + // one. A tokMany could supply zero or many; refuse to guess. + if ii >= len(inner) { + return false + } + switch it := inner[ii]; it.kind { + case tokMany: + return false + case tokAny1: + // Only an unconstrained outer segment covers an arbitrary one. + return ot.kind == tokAny1 && covers(outer, inner, oi+1, ii+1, memo) + default: // tokSeg + if ot.kind != tokAny1 && !segContains(ot.glob, it.glob) { + return false + } + return covers(outer, inner, oi+1, ii+1, memo) + } + }() + memo[key] = r + return r +} + +// segContains reports whether every segment matching glob `inner` also matches +// glob `outer`. Proven only when `inner` is a literal (the case that matters: +// does "build.gradle" fall under "*.gradle"); otherwise only identical globs +// are accepted. +func segContains(outer, inner string) bool { + if outer == inner { + return true + } + lit, ok := literalSeg(inner) + if !ok { + return false + } + re, err := segRegex(outer) + if err != nil { + return false + } + return re.MatchString(lit) +} + +// literalSeg unescapes a segment glob that contains no wildcard, reporting the +// exact string it matches. +func literalSeg(seg string) (string, bool) { + var b strings.Builder + escape := false + for _, ch := range seg { + if escape { + escape = false + b.WriteRune(ch) + continue + } + switch ch { + case '\\': + escape = true + case '*', '?': + return "", false + default: + b.WriteRune(ch) + } + } + if escape { + return "", false + } + return b.String(), true +} + +// segRegex compiles a single segment's glob, using the same escape and +// wildcard rules as buildPatternRegex's default arm (no character classes: +// "[" and "]" are literals). +func segRegex(seg string) (*regexp.Regexp, error) { + var re strings.Builder + re.WriteString(`\A`) + escape := false + for _, ch := range seg { + if escape { + escape = false + re.WriteString(regexp.QuoteMeta(string(ch))) + continue + } + switch ch { + case '\\': + escape = true + case '*': + re.WriteString(`[^/]*`) + case '?': + re.WriteString(`[^/]`) + case '[', ']': + re.WriteString(`\` + string(ch)) + default: + re.WriteString(regexp.QuoteMeta(string(ch))) + } + } + re.WriteString(`\z`) + return regexp.Compile(re.String()) +} diff --git a/internal/pattern/contains_test.go b/internal/pattern/contains_test.go new file mode 100644 index 0000000..3815f4b --- /dev/null +++ b/internal/pattern/contains_test.go @@ -0,0 +1,152 @@ +package pattern_test + +import ( + "strings" + "testing" + + "github.com/jordonpeterson/codeowners-tool/internal/pattern" +) + +// containsCorpus is the pattern universe the brute-force checks range over. It +// deliberately mixes anchoring styles, because the shapes that look equivalent +// but are not ("/docs/" vs "docs/") are exactly where containment goes wrong. +var containsCorpus = []string{ + "*", "**", "/", "*.gradle", "**/*.gradle", "build.gradle", "/build.gradle", + "docs", "docs/", "/docs", "/docs/", "/docs/**", "docs/*", "docs/**", + "docs/*.md", "/docs/*.md", "/docs/readme.md", "**/docs", "**/docs/*", + "x", "x/", "/x", "/x/", "/x/*", "/x/**", "x/y", "/x/y/", "/x/*.gradle", + "x/**/y", "src/", "src/*", "/src/api/", "src/api", "services/**", + "path/app/*", "path/app/", "/path/app/*.gradle", "v?.md", "/a b/*", +} + +// containsPaths is the witness universe. Every path is a plausible repo-relative +// FILE path; several exist only to catch specific traps (a directory whose name +// matches a file glob, the same basename at several depths). +var containsPaths = func() []string { + var out []string + dirs := []string{ + "", "docs/", "x/", "x/y/", "x/y/z/", "src/", "src/api/", "path/", "path/app/", + "a/docs/", "a/b/docs/", "web/docs/", "services/api/", "packages/docs/", + "x/foo.gradle/", "docs/deep/", "a b/", "sub/x/", + } + names := []string{ + "build.gradle", "a.gradle", "readme.md", "v1.md", "main.go", "docs", + "x", "y", "Dockerfile", "a.b.gradle", + } + for _, d := range dirs { + for _, n := range names { + out = append(out, d+n) + } + } + return out +}() + +// Contains must be SOUND: it may answer false when containment holds, but it +// must never answer true when a concrete path matches `inner` and not `outer`. +// An unsound answer here lets the planner amend a rule in place and silently +// hand an owner every future file that rule will ever match — the defect this +// whole mechanism exists to prevent. +func TestContainsIsSound(t *testing.T) { + compiled := map[string]*pattern.Pattern{} + for _, p := range containsCorpus { + c, err := pattern.Compile(p) + if err != nil { + t.Fatalf("Compile(%q): %v", p, err) + } + compiled[p] = c + } + unsound := 0 + for _, outer := range containsCorpus { + for _, inner := range containsCorpus { + if !pattern.Contains(outer, inner) { + continue + } + for _, p := range containsPaths { + if compiled[inner].Match(p) && !compiled[outer].Match(p) { + unsound++ + if unsound <= 20 { + t.Errorf("Contains(%q, %q) = true, but %q matches %q and not %q", + outer, inner, p, inner, outer) + } + break + } + } + } + } + if unsound > 0 { + t.Errorf("%d unsound pair(s)", unsound) + } +} + +// Reflexivity: a pattern always contains itself, whatever its shape. +func TestContainsIsReflexive(t *testing.T) { + for _, p := range containsCorpus { + if !pattern.Contains(p, p) { + t.Errorf("Contains(%q, %q) = false, want true", p, p) + } + } +} + +// The containments the planner relies on to amend a rule in place, and the +// near-miss shapes it must NOT accept. +func TestContainsKnownPairs(t *testing.T) { + cases := []struct { + outer, inner string + want bool + why string + }{ + // A trailing slash does NOT anchor a single-segment pattern: "docs/" + // matches "web/docs/spec.md", which the anchored scope does not. + {"/docs/", "docs/", false, "unanchored rule escapes an anchored scope"}, + {"/docs", "docs/", false, "same, without the scope's trailing slash"}, + {"/docs/**", "docs/", false, "same, with an explicit globstar"}, + {"docs/", "/docs/", true, "the anchored subtree is inside the any-depth one"}, + + // A file glob contains the literals that are instances of it. + {"*.gradle", "build.gradle", true, "literal is an instance of the glob"}, + {"*.gradle", "/build.gradle", true, "anchored literal too"}, + {"*.gradle", "/x/*.gradle", true, "same final segment, deeper"}, + {"*.gradle", "**/*.gradle", true, "**/foo is equivalent to foo"}, + {"*.gradle", "*.md", false, "disjoint globs"}, + {"*.md", "docs/*.md", true, "any-depth glob covers the anchored one"}, + {"/docs/*.md", "/docs/readme.md", true, "glob segment covers the literal"}, + + // A single-segment pattern denotes a whole subtree at any depth. + {"x", "/x/", true, "anchored subtree is inside the any-depth entry"}, + {"x", "/x/**", true, "explicit globstar form"}, + {"x", "/x/*", true, "one-level form"}, + {"/x/", "x", false, "the any-depth entry escapes the anchored subtree"}, + + // Anchoring without a leading slash: a mid-string slash already anchors. + {"src/api", "/src/api/", true, "mid-string slash anchors at the root"}, + {"path/app/", "/path/app/*", true, "same, one level down"}, + {"docs/", "docs/*", true, "one-level rule inside the subtree"}, + {"docs/", "docs/**", true, "globstar rule inside the subtree"}, + + // "*" and "**" are universal; "/" matches nothing. + {"*", "build.gradle", true, "* matches at any depth"}, + {"**", "/x/y/", true, "** is universal"}, + {"/", "*", false, "/ matches nothing"}, + {"*", "/", true, "the empty language is contained in everything"}, + + // One level vs. subtree are genuinely incomparable. + {"/x/*", "/x/", false, "one level does not cover the subtree"}, + {"/x/", "/x/*", true, "subtree covers one level"}, + } + for _, c := range cases { + if got := pattern.Contains(c.outer, c.inner); got != c.want { + t.Errorf("Contains(%q, %q) = %v, want %v — %s", c.outer, c.inner, got, c.want, c.why) + } + } +} + +// Contains must not blow up or report true on patterns Compile rejects. +func TestContainsRejectsInvalid(t *testing.T) { + for _, bad := range []string{"", "!x", "a***b"} { + if pattern.Contains(bad, "x") || pattern.Contains("x", bad) { + if strings.Contains(bad, "!") || bad == "" || strings.Contains(bad, "***") { + t.Errorf("Contains involving invalid pattern %q returned true", bad) + } + } + } +} diff --git a/internal/pattern/pattern.go b/internal/pattern/pattern.go index c5d5b05..db375e5 100644 --- a/internal/pattern/pattern.go +++ b/internal/pattern/pattern.go @@ -76,17 +76,11 @@ func (p *Pattern) Match(testPath string) bool { return p.regex.MatchString(testPath) } -// buildPatternRegex compiles a gitignore-style CODEOWNERS pattern to a regex. -// Ported from hmarr/codeowners. -func buildPatternRegex(pattern string) (*regexp.Regexp, error) { - switch { - case strings.Contains(pattern, "***"): - return nil, fmt.Errorf("pattern cannot contain three consecutive asterisks") - case pattern == "/": - // "/" doesn't match anything - return regexp.Compile(`\A\z`) - } - +// normalizeSegs splits a pattern into the segment list buildPatternRegex +// compiles, applying gitignore's anchoring rules. Contains depends on this +// being the SAME normalization the matcher uses — a divergence here would make +// containment reason about a different language than Match implements. +func normalizeSegs(pattern string) []string { segs := strings.Split(pattern, "/") if segs[0] == "" { @@ -94,7 +88,8 @@ func buildPatternRegex(pattern string) (*regexp.Regexp, error) { segs = segs[1:] } else if len(segs) == 1 || (len(segs) == 2 && segs[1] == "") { // Single-segment pattern with no leading slash matches at any depth - // (equivalent to a leading **/). + // (equivalent to a leading **/). A trailing slash does NOT anchor it: + // "docs/" is one segment and matches "src/docs/notes.md". if segs[0] != "**" { segs = append([]string{"**"}, segs...) } @@ -104,6 +99,21 @@ func buildPatternRegex(pattern string) (*regexp.Regexp, error) { // Trailing slash is equivalent to "/**". segs[len(segs)-1] = "**" } + return segs +} + +// buildPatternRegex compiles a gitignore-style CODEOWNERS pattern to a regex. +// Ported from hmarr/codeowners. +func buildPatternRegex(pattern string) (*regexp.Regexp, error) { + switch { + case strings.Contains(pattern, "***"): + return nil, fmt.Errorf("pattern cannot contain three consecutive asterisks") + case pattern == "/": + // "/" doesn't match anything + return regexp.Compile(`\A\z`) + } + + segs := normalizeSegs(pattern) const sep = "/" lastSegIndex := len(segs) - 1 diff --git a/internal/plan/plan.go b/internal/plan/plan.go index f47b416..70a2284 100644 --- a/internal/plan/plan.go +++ b/internal/plan/plan.go @@ -408,7 +408,6 @@ func simulate(op ops.Op, owners []string) []string { func synthAdd(f *file.File, tree []string, op ops.Op, scope map[string]bool, desired map[string][]string, pl *Plan) error { cur := resolve.All(f, tree) - winners := winnersByLine(cur) warnShadowedDuplicates(f, tree, scope, pl) groups := map[int]bool{} @@ -432,7 +431,7 @@ func synthAdd(f *file.File, tree []string, op ops.Op, scope map[string]bool, des if contains(r.Owners, op.Owner) { continue } - if subset(winners[l], scope) { + if pattern.Contains(op.Scope, r.PatternText) { old := f.LineText(l) // Capture BEFORE SetOwners mutates the rule — r aliases the live // rule, so a post-mutation OwnersCopy reports the new set as the @@ -444,15 +443,18 @@ func synthAdd(f *file.File, tree []string, op ops.Op, scope map[string]bool, des Action: "amend", Line: l + 1, Pattern: r.PatternText, OldOwners: oldOwners, NewOwners: newOwners, OldLine: old, NewLine: f.LineText(l), - Reason: fmt.Sprintf("every path governed by %q is inside scope %q; amended in place (R-2/R-4)", r.PatternText, op.Scope), + Reason: fmt.Sprintf("pattern %q can only ever match paths inside scope %q; amended in place (R-2/R-4)", r.PatternText, op.Scope), }) } else { - inter, ok := intersectPattern(op.Scope, r, scope, tree) + inter, exact, ok := intersectPattern(op.Scope, r, scope, tree) if !ok { return &RefusalError{Msg: fmt.Sprintf( "refusing: rule %q also governs paths outside scope %q, and no sound narrowing pattern is derivable — amending would violate INV-2, appending would violate INV-1", r.PatternText, op.Scope)} } + if !exact { + pl.addWarning(inexactNarrowingWarning(inter, op.Scope, r.PatternText)) + } newOwners := append(append([]string{}, r.Owners...), op.Owner) f.InsertRule(l+1, inter, newOwners) pl.Changes = append(pl.Changes, Change{ @@ -525,7 +527,12 @@ func synthSet(f *file.File, tree []string, op ops.Op, scope map[string]bool, des } } - if lastRule != nil && matchSetEquals(lastRule, tree, scope) { + // Amend only when the rule governs EXACTLY the scope as a pattern. Equal + // match sets over the tracked tree are not enough: set_owners replaces the + // owner set outright, so amending a rule that merely looks scope-sized + // today hands every future file it matches to the new owners and strips + // whoever owned them (review finding — the reported bug, in set_owners). + if lastRule != nil && samePatternLanguage(op.Scope, lastRule.PatternText) { if resolve.OwnersEqual(lastRule.OwnersCopy(), op.Owners) { // Already exact; nothing to do for this op. } else { @@ -622,7 +629,6 @@ func synthRemove(f *file.File, tree []string, op ops.Op, scope map[string]bool, // was made. func removePass(f *file.File, tree []string, op ops.Op, scope map[string]bool, onEmpty string, pl *Plan) (bool, error) { cur := resolve.All(f, tree) - winners := winnersByLine(cur) groups := map[int][]string{} for p := range scope { @@ -642,7 +648,7 @@ func removePass(f *file.File, tree []string, op ops.Op, scope map[string]bool, o for _, l := range lines { r := f.Lines[l].Rule newOwners := minus(r.Owners, op.Owner) - if subset(winners[l], scope) { + if pattern.Contains(op.Scope, r.PatternText) { if len(newOwners) > 0 { old := f.LineText(l) oldOwners := r.OwnersCopy() @@ -651,7 +657,7 @@ func removePass(f *file.File, tree []string, op ops.Op, scope map[string]bool, o Action: "amend", Line: l + 1, Pattern: r.PatternText, OldOwners: oldOwners, NewOwners: newOwners, OldLine: old, NewLine: f.LineText(l), - Reason: fmt.Sprintf("every path governed by %q is inside scope; removed %s in place", r.PatternText, op.Owner), + Reason: fmt.Sprintf("pattern %q can only ever match paths inside scope; removed %s in place", r.PatternText, op.Owner), }) continue } @@ -687,11 +693,14 @@ func removePass(f *file.File, tree []string, op ops.Op, scope map[string]bool, o } } else { // Rule also governs out-of-scope paths: split via narrowing insert. - inter, ok := intersectPattern(op.Scope, r, scope, tree) + inter, exact, ok := intersectPattern(op.Scope, r, scope, tree) if !ok { return false, &RefusalError{Msg: fmt.Sprintf( "refusing: rule %q also governs paths outside scope %q and no sound narrowing pattern is derivable", r.PatternText, op.Scope)} } + if !exact { + pl.addWarning(inexactNarrowingWarning(inter, op.Scope, r.PatternText)) + } if len(newOwners) == 0 { switch onEmpty { case "": @@ -753,6 +762,109 @@ func synthRename(f *file.File, op ops.Op, desired map[string][]string, pl *Plan) return nil } +// intersectPattern derives a pattern matching exactly (scope ∩ rule) and +// PROVES it before returning. deriveIntersection supplies the candidate +// shapes; this layer is what makes them trustworthy. +// +// The load-bearing check is pattern containment: the candidate must not reach +// outside the scope (which would grant the new owner beyond what was asked) +// nor outside the rule (which would grant the RULE's owners paths it never +// governed). Both are claims about files that do not exist yet, so no tree can +// establish them. +// +// exact=false means the candidate is right for every tracked file but not +// provably confined for future ones — the caller discloses it. CODEOWNERS +// genuinely cannot express some intersections: a `dir/*` rule governs one +// level, and no pattern says "one level AND matching *.gradle", since +// `dir/*.gradle` also matches under a DIRECTORY named `.gradle`. Refusing +// outright would make add_owner(*.gradle, …) impossible on any repo with a +// `dir/*` rule. +func intersectPattern(scope string, rule *file.Rule, scopeSet map[string]bool, tree []string) (inter string, exact bool, ok bool) { + var cands []string + if c, got := deriveIntersection(scope, rule, scopeSet, tree); got { + cands = append(cands, c) + } + // deriveIntersection rejects one-level (`dir/*`) rules outright, since no + // subtree prefix describes them. The closest expressible narrowing is the + // glob at that same level; it is inexact only for descendants of a + // directory whose own name matches the glob. + if seg, got := basenameGlob(scope); got { + if d, got := oneLevelRuleDir(rule.PatternText); got { + cands = append(cands, d+seg) + } + } + for _, c := range cands { + if treeExact(c, rule, scopeSet, tree) && + pattern.Contains(scope, c) && pattern.Contains(rule.PatternText, c) { + return c, true, true + } + } + for _, c := range cands { + if treeExact(c, rule, scopeSet, tree) { + return c, false, true + } + } + return "", false, false +} + +// oneLevelRuleDir returns the directory prefix of a rule that governs a +// directory's DIRECT CHILDREN (`path/app/*`), preserving the rule's own +// anchoring so the derived pattern matches at the same depth. +func oneLevelRuleDir(pat string) (string, bool) { + if pat == "*" || !strings.HasSuffix(pat, "/*") || strings.HasSuffix(pat, "/**/*") { + return "", false + } + return strings.TrimSuffix(pat, "*"), true +} + +// treeExact reports whether a candidate matches exactly (scope ∩ rule) over the +// tracked tree. It pins down an under-narrow guess with a clear refusal instead +// of letting it loop in removePass's fixpoint or surface as a gate violation, +// but says nothing about files that do not exist yet. +func treeExact(cand string, rule *file.Rule, scopeSet map[string]bool, tree []string) bool { + cp, err := pattern.Compile(cand) + if err != nil { + return false + } + for _, p := range tree { + if cp.Match(p) != (scopeSet[p] && rule.Pattern.Match(p)) { + return false + } + } + return true +} + +// basenameGlob recognizes a single-segment pattern — one that matches a +// basename at any depth. +func basenameGlob(pat string) (string, bool) { + if pat == "" || pat == "**" || strings.Contains(pat, "/") { + return "", false + } + return pat, true +} + +// samePatternLanguage reports whether two patterns match exactly the same set +// of paths. +func samePatternLanguage(a, b string) bool { + return pattern.Contains(a, b) && pattern.Contains(b, a) +} + +// inexactNarrowingWarning explains a narrowing rule that is exact for every +// tracked file but not provably exact for files that do not exist yet. +func inexactNarrowingWarning(inter, scope, rulePat string) string { + return fmt.Sprintf( + "narrowing rule %q is exact for every tracked file, but is not provably confined to %q ∩ %q for files added later; "+ + "a future path matching %q that %q does not govern would also pick up that rule's owners", + inter, scope, rulePat, inter, rulePat) +} + +// addWarning appends a warning, skipping exact duplicates. +func (p *Plan) addWarning(w string) { + if !containsStr(p.Warnings, w) { + p.Warnings = append(p.Warnings, w) + } +} + // intersectPattern derives a pattern matching exactly (scope ∩ rule) for the // shapes that arise in practice. Returns ok=false when no sound derivation // exists — the caller refuses rather than guesses; the gate re-proves @@ -777,7 +889,7 @@ func synthRename(f *file.File, op ops.Op, desired map[string][]string, pl *Plan) // derivation can under-match (a rule whose own last segment matches the // scope glob, e.g. /app* × *.gradle over a root file apple.gradle), so it // is confirmed against the tree before being returned. -func intersectPattern(scope string, rule *file.Rule, scopeSet map[string]bool, tree []string) (string, bool) { +func deriveIntersection(scope string, rule *file.Rule, scopeSet map[string]bool, tree []string) (string, bool) { subset := true for p := range scopeSet { if !rule.Pattern.Match(p) { @@ -949,16 +1061,6 @@ func warnShadowedDuplicates(f *file.File, tree []string, scope map[string]bool, } } -func winnersByLine(res map[string]resolve.Resolution) map[int][]string { - out := map[int][]string{} - for p, r := range res { - if r.Matched { - out[r.LineIndex] = append(out[r.LineIndex], p) - } - } - return out -} - func firstRuleIndex(f *file.File) int { for i, ln := range f.Lines { if ln.Kind == file.LineRule { @@ -968,28 +1070,6 @@ func firstRuleIndex(f *file.File) int { return len(f.Lines) } -func matchSetEquals(r *file.Rule, tree []string, scope map[string]bool) bool { - n := 0 - for _, p := range tree { - if r.Pattern.Match(p) { - if !scope[p] { - return false - } - n++ - } - } - return n == len(scope) -} - -func subset(paths []string, set map[string]bool) bool { - for _, p := range paths { - if !set[p] { - return false - } - } - return true -} - func contains(list []string, s string) bool { for _, x := range list { if x == s { diff --git a/internal/plan/plan_test.go b/internal/plan/plan_test.go index 7bb6c1f..6f485f9 100644 --- a/internal/plan/plan_test.go +++ b/internal/plan/plan_test.go @@ -511,6 +511,281 @@ func TestR6_UnownedPolicyPureTransform(t *testing.T) { } } +// A file-glob scope must never widen a directory rule. The amend-in-place +// decision used to be a SNAPSHOT property ("every tracked file this rule wins +// is in scope"), but a CODEOWNERS rule governs files that do not exist yet. +// With build.gradle the only tracked file under path/app/, the planner amended +// `path/app/* @b` to `path/app/* @b @gradle` — handing @gradle every future +// non-gradle file in that directory. It must insert a narrowing +// `path/app/*.gradle` rule instead. (Reported by a user.) +func TestR2_FileGlobScopeDoesNotWidenDirectoryRule(t *testing.T) { + tree := []string{"build.gradle", "path/app/build.gradle"} + p, err := build(t, "* @a\npath/app/* @b\n", tree, plan.Options{}, "add_owner(*.gradle, @gradle)") + if err != nil { + t.Fatal(err) + } + for _, c := range p.Changes { + if c.Action == "amend" && c.Pattern == "path/app/*" { + t.Errorf("amended the directory rule %q instead of narrowing it: %+v", c.Pattern, c) + } + } + after := plan.ResolveContent(p.AfterContent, tree) + got := after["path/app/build.gradle"].Owners + sort.Strings(got) + if !reflect.DeepEqual(got, []string{"@b", "@gradle"}) { + t.Errorf("path/app/build.gradle = %v, want {@b, @gradle}", got) + } + got = after["build.gradle"].Owners + sort.Strings(got) + if !reflect.DeepEqual(got, []string{"@a", "@gradle"}) { + t.Errorf("build.gradle = %v, want {@a, @gradle}", got) + } + + // The point of the fix: a non-gradle file added to path/app/ LATER must + // still resolve to @b alone. The tracked tree cannot show this, so resolve + // the planned content against a tree that includes the future file. + future := append(append([]string{}, tree...), "path/app/main.kt") + got = plan.ResolveContent(p.AfterContent, future)["path/app/main.kt"].Owners + if !reflect.DeepEqual(got, []string{"@b"}) { + t.Errorf("future path/app/main.kt = %v, want {@b} — the gradle owner must not inherit the directory", got) + } +} + +// Same snapshot trap for the other common directory shape: an anchored rule +// with no trailing slash, whose match set is a whole subtree. +func TestR2_FileGlobScopeDoesNotWidenAnchoredDirRule(t *testing.T) { + tree := []string{"build.gradle", "svc/build.gradle"} + p, err := build(t, "* @a\n/svc @b\n", tree, plan.Options{}, "add_owner(*.gradle, @gradle)") + if err != nil { + t.Fatal(err) + } + for _, c := range p.Changes { + if c.Action == "amend" && c.Pattern == "/svc" { + t.Errorf("amended the directory rule %q instead of narrowing it: %+v", c.Pattern, c) + } + } + after := plan.ResolveContent(p.AfterContent, tree) + got := after["svc/build.gradle"].Owners + sort.Strings(got) + if !reflect.DeepEqual(got, []string{"@b", "@gradle"}) { + t.Errorf("svc/build.gradle = %v, want {@b, @gradle}", got) + } + future := append(append([]string{}, tree...), "svc/deep/main.kt") + got = plan.ResolveContent(p.AfterContent, future)["svc/deep/main.kt"].Owners + if !reflect.DeepEqual(got, []string{"@b"}) { + t.Errorf("future svc/deep/main.kt = %v, want {@b}", got) + } +} + +// remove_owner shares the amend decision, so it shared the bug: removing +// @b for *.gradle must not strip @b from the whole directory. +func TestR2_FileGlobScopeDoesNotNarrowDirectoryRuleOnRemove(t *testing.T) { + tree := []string{"path/app/build.gradle"} + p, err := build(t, "* @a\npath/app/* @b @gradle\n", tree, plan.Options{}, + "remove_owner(*.gradle, @gradle)") + if err != nil { + t.Fatal(err) + } + for _, c := range p.Changes { + if c.Action == "amend" && c.Pattern == "path/app/*" { + t.Errorf("amended the directory rule %q instead of narrowing it: %+v", c.Pattern, c) + } + } + future := append(append([]string{}, tree...), "path/app/main.kt") + after := plan.ResolveContent(p.AfterContent, future) + if got := after["path/app/build.gradle"].Owners; !reflect.DeepEqual(got, []string{"@b"}) { + t.Errorf("path/app/build.gradle = %v, want {@b}", got) + } + got := after["path/app/main.kt"].Owners + sort.Strings(got) + if !reflect.DeepEqual(got, []string{"@b", "@gradle"}) { + t.Errorf("future path/app/main.kt = %v, want {@b, @gradle} — a non-gradle file must keep @gradle", got) + } +} + +// Review finding: a trailing slash does NOT anchor a single-segment pattern. +// `docs/` is one segment, so gitignore gives it an implicit leading `**/` and +// it matches `web/docs/spec.md`. A textual prefix comparison read it as +// root-anchored and amended it in place for the scope `/docs/`, handing @x +// every `docs` directory in the repo — the reported bug in a shape the first +// fix did not cover. +func TestR2_UnanchoredDirectoryRuleIsNotAmendedForAnchoredScope(t *testing.T) { + tree := []string{"docs/readme.md", "src/x.go"} + p, err := build(t, "* @a\ndocs/ @b\n", tree, plan.Options{}, "add_owner(/docs/, @x)") + if err != nil { + t.Fatal(err) + } + for _, c := range p.Changes { + if c.Action == "amend" && c.Pattern == "docs/" { + t.Errorf("amended unanchored rule %q for anchored scope /docs/: %+v", c.Pattern, c) + } + } + after := plan.ResolveContent(p.AfterContent, tree) + got := after["docs/readme.md"].Owners + sort.Strings(got) + if !reflect.DeepEqual(got, []string{"@b", "@x"}) { + t.Errorf("docs/readme.md = %v, want {@b, @x}", got) + } + // The whole point: a docs directory somewhere else is outside /docs/. + future := append(append([]string{}, tree...), "web/docs/spec.md") + got = plan.ResolveContent(p.AfterContent, future)["web/docs/spec.md"].Owners + if !reflect.DeepEqual(got, []string{"@b"}) { + t.Errorf("future web/docs/spec.md = %v, want {@b} — it is outside scope /docs/", got) + } +} + +// Containment must be semantic, not textual. Each of these rules is genuinely +// inside its op's scope, so amending in place is sound and the planner must not +// refuse or bloat the file. A textual last-segment comparison rejected them all. +func TestR2_ContainmentIsSemanticNotTextual(t *testing.T) { + cases := []struct { + name, content, op string + tree []string + wantAfter string + }{ + {"literal is an instance of the scope glob", + "* @a\nbuild.gradle @b\n", "add_owner(*.gradle, @g)", + []string{"build.gradle", "x/build.gradle"}, "* @a\nbuild.gradle @b @g\n"}, + {"anchored literal under a basename glob", + "* @a\n/build.gradle @b\n", "add_owner(*.gradle, @g)", + []string{"build.gradle", "svc/x.gradle"}, "* @a\n*.gradle @a @g\n/build.gradle @b @g\n"}, + {"mid-string slash already anchors the scope", + "* @a\npath/app/* @b\n", "add_owner(path/app/, @x)", + []string{"path/app/build.gradle"}, "* @a\npath/app/* @b @x\n"}, + {"directory rule inside a bare directory-name scope", + "* @a\ndocs/ @b\n", "add_owner(docs, @d)", + []string{"docs/readme.md"}, "* @a\ndocs/ @b @d\n"}, + {"anchored subtree rule inside an unanchored equivalent", + "* @a\n/src/api/ @b\n", "add_owner(src/api, @w)", + []string{"src/api/a.go", "src/other.go"}, "* @a\n/src/api/ @b @w\n"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + p, err := build(t, c.content, c.tree, plan.Options{}, c.op) + if err != nil { + t.Fatalf("%s must not refuse: %v", c.op, err) + } + if p.AfterContent != c.wantAfter { + t.Errorf("after = %q, want %q", p.AfterContent, c.wantAfter) + } + }) + } +} + +// Review finding: a `dir/*` rule governs exactly ONE level, but no CODEOWNERS +// pattern says "one level AND matching *.gradle" — `path/app/*.gradle` also +// matches files under a DIRECTORY named `.gradle`. The narrowing rule is exact +// for every tracked file, so it is emitted, but the residual must be disclosed +// rather than silently presented as proven. +func TestR2_InexactNarrowingIsDisclosed(t *testing.T) { + tree := []string{"build.gradle", "path/app/build.gradle"} + p, err := build(t, "* @a\npath/app/* @b\n", tree, plan.Options{}, "add_owner(*.gradle, @g)") + if err != nil { + t.Fatal(err) + } + if want := "* @a\n*.gradle @a @g\npath/app/* @b\npath/app/*.gradle @b @g\n"; p.AfterContent != want { + t.Errorf("after = %q, want %q", p.AfterContent, want) + } + var found bool + for _, w := range p.Warnings { + if strings.Contains(w, `"path/app/*.gradle"`) && strings.Contains(w, "not provably confined") { + found = true + } + } + if !found { + t.Errorf("expected a warning disclosing the inexact narrowing, got %v", p.Warnings) + } + // A rule whose reach IS expressible must be proven, not warned about. + p2, err := build(t, "* @a\n/svc @b\n", []string{"build.gradle", "svc/build.gradle"}, + plan.Options{}, "add_owner(*.gradle, @g)") + if err != nil { + t.Fatal(err) + } + if want := "* @a\n*.gradle @a @g\n/svc @b\n/svc/**/*.gradle @b @g\n"; p2.AfterContent != want { + t.Errorf("after = %q, want %q", p2.AfterContent, want) + } + if len(p2.Warnings) != 0 { + t.Errorf("subtree rule narrowing is provably exact; want no warnings, got %v", p2.Warnings) + } +} + +// Review finding: a single-segment scope that is a plain directory NAME (`x`, +// `docs`) denotes a whole subtree, not a filename glob. Deriving a narrowing +// pattern as though it were a glob produced junk (`x/**/x`) that matched none +// of the group's paths, so removePass re-inserted it every pass and then +// reported "did not converge — file structure defeats the writer" for a +// two-line file. The rule is inside the scope, so it must simply be amended. +func TestR6_DirectoryNameScopeConverges(t *testing.T) { + cases := []struct { + name, content, op string + tree []string + wantAfter string + }{ + {"anchored globstar rule", "* @a\n/x/** @a @c\n", "remove_owner(x, @c)", + []string{"x/sub/c.gradle", "x/main.go"}, "* @a\n/x/** @a\n"}, + {"unanchored directory rule", "* @a\ndocs/ @b @d\n", "remove_owner(docs, @d)", + []string{"docs/readme.md"}, "* @a\ndocs/ @b\n"}, + } + for _, c := range cases { + for _, policy := range []string{"", "error", "inherit", "unowned"} { + t.Run(c.name+"/"+policy, func(t *testing.T) { + p, err := build(t, c.content, c.tree, plan.Options{OnEmpty: policy}, c.op) + if err != nil { + t.Fatalf("must not refuse (--on-empty=%q): %v", policy, err) + } + if p.AfterContent != c.wantAfter { + t.Errorf("after = %q, want %q", p.AfterContent, c.wantAfter) + } + }) + } + } +} + +// set_owners shared the snapshot bug: it amended the last intersecting rule +// whenever that rule's TRACKED match set equalled the scope set. With +// svc/sub/a.go the only tracked file under /svc/, it rewrote `/svc/ @b` to +// `/svc/ @g` — silently transferring the entire /svc/ tree away from @b. +func TestR4_SetOwnersDoesNotAmendABroaderRule(t *testing.T) { + tree := []string{"svc/sub/a.go", "README.md"} + p, err := build(t, "* @a\n/svc/ @b\n", tree, plan.Options{}, "set_owners(/svc/sub/, [@g])") + if err != nil { + t.Fatal(err) + } + for _, c := range p.Changes { + if c.Action == "amend" && c.Pattern == "/svc/" { + t.Errorf("amended the broader rule %q: %+v", c.Pattern, c) + } + } + if got := plan.ResolveContent(p.AfterContent, tree)["svc/sub/a.go"].Owners; !reflect.DeepEqual(got, []string{"@g"}) { + t.Errorf("svc/sub/a.go = %v, want {@g}", got) + } + future := append(append([]string{}, tree...), "svc/other.go") + got := plan.ResolveContent(p.AfterContent, future)["svc/other.go"].Owners + if !reflect.DeepEqual(got, []string{"@b"}) { + t.Errorf("future svc/other.go = %v, want {@b} — @b must keep the rest of /svc/", got) + } +} + +// The same, in the shape the user reported: a file-glob scope must not rewrite +// a directory rule's owner set wholesale. +func TestR2_FileGlobScopeDoesNotWidenDirectoryRuleOnSet(t *testing.T) { + tree := []string{"path/app/build.gradle", "README.md"} + p, err := build(t, "* @a\npath/app/* @b\n", tree, plan.Options{}, "set_owners(*.gradle, [@g])") + if err != nil { + t.Fatal(err) + } + for _, c := range p.Changes { + if c.Action == "amend" && c.Pattern == "path/app/*" { + t.Errorf("amended the directory rule %q: %+v", c.Pattern, c) + } + } + future := append(append([]string{}, tree...), "path/app/main.kt") + got := plan.ResolveContent(p.AfterContent, future)["path/app/main.kt"].Owners + if !reflect.DeepEqual(got, []string{"@b"}) { + t.Errorf("future path/app/main.kt = %v, want {@b}", got) + } +} + // E2E-testing finding: add_owner amend records recorded the POST-op owner // set as old_owners (OwnersCopy taken after SetOwners mutated the aliased // rule). The change record must show the true before/after.