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.