diff --git a/codeowners.toml b/codeowners.toml index a859ab6..41f0a17 100644 --- a/codeowners.toml +++ b/codeowners.toml @@ -5,7 +5,7 @@ max_reviews = 2 # `unskippable_reviewers` allows you to specify reviewers that cannot be skipped via the max_reviews setting unskippable_reviewers = ["@BakerNet"] # `ignore` allows you to specify directories that should be ignored by the codeowners check -ignore = ["test_project"] +ignore = ["test_project", "test_project_inline"] # `high_priority_lables` allows you to specify labels that should be considered high priority high_priority_labels = ["P0"] @@ -16,3 +16,5 @@ high_priority_labels = ["P0"] approval = false # `fail_check` (default true) means the codeowners GHA check will fail if the codeowners check fails fail_check = true +# `inline_ownership_enabled` (default false) enables parsing of inline ownership tags in source files +inline_ownership_enabled = true diff --git a/internal/app/app.go b/internal/app/app.go index 4e70994..554204c 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -3,6 +3,8 @@ package app import ( "fmt" "io" + "os" + "path/filepath" "slices" "strings" "time" @@ -12,6 +14,7 @@ import ( gh "github.com/multimediallc/codeowners-plus/internal/github" "github.com/multimediallc/codeowners-plus/pkg/codeowners" f "github.com/multimediallc/codeowners-plus/pkg/functional" + "github.com/multimediallc/codeowners-plus/pkg/inlineowners" ) // Config holds the application configuration @@ -101,18 +104,70 @@ func (a *App) Run() (bool, string, error) { } a.codeowners = codeOwners + // Inline ownership integration + if a.Conf != nil && a.Conf.InlineOwnershipEnabled { + oracle := inlineowners.Oracle{} + // build oracle blocks per file + for _, df := range gitDiff.AllChanges() { + abs := filepath.Join(a.config.RepoDir, df.FileName) + data, err := os.ReadFile(abs) + if err != nil { + a.printWarn("WARNING: unable to read file %s: %v\n", df.FileName, err) + continue + } + blks, _ := inlineowners.Parse(string(data), a.config.WarningBuffer) + if len(blks) > 0 { + // convert to Block type + b2 := make([]inlineowners.Block, 0, len(blks)) + for _, pb := range blks { + b2 = append(b2, inlineowners.Block{Owners: pb.Owners, Start: pb.StartLine, End: pb.EndLine}) + } + oracle[df.FileName] = b2 + } + } + + overrides := make(map[string]codeowners.ReviewerGroups) + for _, df := range gitDiff.AllChanges() { + // aggregate owners from hunks via oracle + rgs := codeowners.ReviewerGroups{} + for _, h := range df.Hunks { + lists := oracle.OwnersForRange(df.FileName, h.Start, h.End) + if lists == nil { + continue + } + for _, lst := range lists { + rgs = append(rgs, &codeowners.ReviewerGroup{Names: lst, Approved: false}) + } + } + if len(rgs) == 0 { + // fallback to file-level + if baseGroups, ok := codeOwners.FileRequired()[df.FileName]; ok { + rgs = append(rgs, baseGroups...) + } + } else { + rgs = f.RemoveDuplicates(rgs) + } + overrides[df.FileName] = rgs + } + + a.codeowners = newOverlayOwners(codeOwners, overrides) + } else { + // feature disabled, keep original codeOwners + a.codeowners = codeOwners + } + // Set author author := fmt.Sprintf("@%s", a.client.PR().User.GetLogin()) - codeOwners.SetAuthor(author) + a.codeowners.SetAuthor(author) // Warn about unowned files - for _, uFile := range codeOwners.UnownedFiles() { + for _, uFile := range a.codeowners.UnownedFiles() { a.printWarn("WARNING: Unowned File: %s\n", uFile) } // Print file owners if verbose if a.config.Verbose { - a.printFileOwners(codeOwners) + a.printFileOwners(a.codeowners) } // Process approvals and reviewers diff --git a/internal/app/app_test.go b/internal/app/app_test.go index 46de8af..115ee60 100644 --- a/internal/app/app_test.go +++ b/internal/app/app_test.go @@ -108,6 +108,8 @@ func (m *mockCodeOwners) UnownedFiles() []string { return m.unownedFiles } +func (m *mockCodeOwners) AddInlineOwners(file string, owners codeowners.ReviewerGroups) {} + type mockGitHubClient struct { pr *github.PullRequest userReviewerMapError error diff --git a/internal/app/inline_adapter.go b/internal/app/inline_adapter.go new file mode 100644 index 0000000..1586848 --- /dev/null +++ b/internal/app/inline_adapter.go @@ -0,0 +1,69 @@ +package app + +import ( + "slices" + + "github.com/multimediallc/codeowners-plus/pkg/codeowners" + f "github.com/multimediallc/codeowners-plus/pkg/functional" +) + +// overlayOwners implements codeowners.CodeOwners by overriding the required reviewer +// groups for specific files. All other behaviour is delegated to the base object. +// Only precedence for required owners is currently affected. + +type overlayOwners struct { + base codeowners.CodeOwners + required map[string]codeowners.ReviewerGroups +} + +func newOverlayOwners(base codeowners.CodeOwners, overrides map[string]codeowners.ReviewerGroups) codeowners.CodeOwners { + return &overlayOwners{base: base, required: overrides} +} + +// SetAuthor propagates to base and ensures author removal from override groups. +func (o *overlayOwners) SetAuthor(author string) { + o.base.SetAuthor(author) + for _, groups := range o.required { + for _, g := range groups { + g.Names = f.RemoveValue(g.Names, author) + if len(g.Names) == 0 { + g.Approved = true + } + } + } +} + +func (o *overlayOwners) FileRequired() map[string]codeowners.ReviewerGroups { return o.required } + +func (o *overlayOwners) FileOptional() map[string]codeowners.ReviewerGroups { + return o.base.FileOptional() +} + +func (o *overlayOwners) AllRequired() codeowners.ReviewerGroups { + agg := make(codeowners.ReviewerGroups, 0) + for _, grps := range o.required { + agg = append(agg, grps...) + } + return f.RemoveDuplicates(agg) +} + +func (o *overlayOwners) AllOptional() codeowners.ReviewerGroups { return o.base.AllOptional() } + +func (o *overlayOwners) UnownedFiles() []string { return o.base.UnownedFiles() } + +func (o *overlayOwners) ApplyApprovals(approvers []string) { + o.base.ApplyApprovals(approvers) + for _, a := range approvers { + for _, groups := range o.required { + for _, g := range groups { + if slices.Contains(g.Names, a) { + g.Approved = true + } + } + } + } +} + +func (o *overlayOwners) AddInlineOwners(file string, owners codeowners.ReviewerGroups) { + // Not used; overrides already applied. +} diff --git a/internal/app/inline_integration_test.go b/internal/app/inline_integration_test.go new file mode 100644 index 0000000..ca1cf9e --- /dev/null +++ b/internal/app/inline_integration_test.go @@ -0,0 +1,63 @@ +package app + +import ( + "os" + "path/filepath" + "testing" + + ownersConfig "github.com/multimediallc/codeowners-plus/internal/config" + "github.com/multimediallc/codeowners-plus/pkg/codeowners" + "github.com/multimediallc/codeowners-plus/pkg/inlineowners" +) + +func TestInlinePrecedence_EndToEnd(t *testing.T) { + tmp := t.TempDir() + // create .codeowners with file-level owner + if err := os.WriteFile(filepath.Join(tmp, ".codeowners"), []byte("a.go @file\n"), 0644); err != nil { + t.Fatalf("write codeowners: %v", err) + } + // create a.go with inline block lines 1-3 + src := `// +line +// +` + if err := os.WriteFile(filepath.Join(tmp, "a.go"), []byte(src), 0644); err != nil { + t.Fatalf("write a.go: %v", err) + } + + diffFile := codeowners.DiffFile{ + FileName: "a.go", + Hunks: []codeowners.HunkRange{{Start: 2, End: 2}}, // line inside block + } + + // Build base codeowners + baseCO, err := codeowners.New(tmp, []codeowners.DiffFile{diffFile}, os.Stdout) + if err != nil { + t.Fatalf("codeowners.New err: %v", err) + } + + // Build oracle + blocks, _ := inlineowners.Parse(src, os.Stdout) + oracle := inlineowners.Oracle{"a.go": { + {Owners: blocks[0].Owners, Start: blocks[0].StartLine, End: blocks[0].EndLine}, + }} + + // Build overrides like app logic + lists := oracle.OwnersForRange("a.go", 2, 2) + overrides := map[string]codeowners.ReviewerGroups{} + rgs := codeowners.ReviewerGroups{} + for _, lst := range lists { + rgs = append(rgs, &codeowners.ReviewerGroup{Names: lst}) + } + overrides["a.go"] = rgs + + ov := newOverlayOwners(baseCO, overrides) + + conf := &ownersConfig.Config{InlineOwnershipEnabled: true} + _ = conf // unused but indicates enable flag + + req := ov.AllRequired().Flatten() + if len(req) != 1 || req[0] != "@inline" { + t.Fatalf("expected only @inline, got %v", req) + } +} diff --git a/internal/app/inline_repo_integration_test.go b/internal/app/inline_repo_integration_test.go new file mode 100644 index 0000000..3b8043c --- /dev/null +++ b/internal/app/inline_repo_integration_test.go @@ -0,0 +1,138 @@ +package app + +import ( + "io" + "os" + "path/filepath" + "reflect" + "slices" + "strings" + "testing" + + "github.com/multimediallc/codeowners-plus/pkg/codeowners" + f "github.com/multimediallc/codeowners-plus/pkg/functional" + "github.com/multimediallc/codeowners-plus/pkg/inlineowners" + "github.com/sourcegraph/go-diff/diff" +) + +// TestInlineRepoFixture_EndToEnd ensures that inline ownership blocks in the +// test_project_inline fixture correctly override file-level CODEOWNERS rules +// for the changed lines contained in the diff fixture. +func TestInlineRepoFixture_EndToEnd(t *testing.T) { + // Location of the fixture relative to this test file (internal/app) + repoDir := filepath.Join("..", "..", "test_project_inline") + + // 1. Load and parse the diff fixture ------------------------------------------------- + diffBytes, err := os.ReadFile(filepath.Join(repoDir, ".diff_changes")) + if err != nil { + t.Fatalf("read diff fixture: %v", err) + } + + parsed, err := diff.ParseMultiFileDiff(diffBytes) + if err != nil { + t.Fatalf("parse diff: %v", err) + } + + // Convert to []codeowners.DiffFile (simplified in-place variant of git.toDiffFiles) + diffFiles := make([]codeowners.DiffFile, 0, len(parsed)) + for _, fd := range parsed { + // Tolerate malformed diffs from the test fixture + if fd.NewName == "/dev/null" || len(fd.NewName) < 3 { + continue + } + df := codeowners.DiffFile{ + FileName: strings.TrimPrefix(fd.NewName[2:], "test_project_inline/"), // strip leading "b/" & repo dir + Hunks: make([]codeowners.HunkRange, 0, len(fd.Hunks)), + } + for _, h := range fd.Hunks { + hr := codeowners.HunkRange{ + Start: int(h.NewStartLine), + End: int(h.NewStartLine + h.NewLines - 1), + } + df.Hunks = append(df.Hunks, hr) + } + diffFiles = append(diffFiles, df) + } + + // 2. Build the base CODEOWNERS model ------------------------------------------------- + baseCO, err := codeowners.New(repoDir, diffFiles, io.Discard) + if err != nil { + t.Fatalf("codeowners.New err: %v", err) + } + + // 3. Build an inline ownership oracle for all changed files -------------------------- + oracle := inlineowners.Oracle{} + for _, df := range diffFiles { + absPath := filepath.Join(repoDir, df.FileName) + data, err := os.ReadFile(absPath) + if err != nil { + t.Fatalf("read file %s: %v", df.FileName, err) + } + blocks, _ := inlineowners.Parse(string(data), io.Discard) + if len(blocks) == 0 { + continue + } + b2 := make([]inlineowners.Block, 0, len(blocks)) + for _, b := range blocks { + b2 = append(b2, inlineowners.Block{Owners: b.Owners, Start: b.StartLine, End: b.EndLine}) + } + oracle[df.FileName] = b2 + } + + // 4. Compute per-file override reviewer groups based on inline blocks --------------- + overrides := map[string]codeowners.ReviewerGroups{} + for _, df := range diffFiles { + rgs := codeowners.ReviewerGroups{} + for _, h := range df.Hunks { + lists := oracle.OwnersForRange(df.FileName, h.Start, h.End) + for _, lst := range lists { + rgs = append(rgs, &codeowners.ReviewerGroup{Names: lst}) + } + } + if len(rgs) == 0 { + if base, ok := baseCO.FileRequired()[df.FileName]; ok { + rgs = append(rgs, base...) + } + } else { + rgs = f.RemoveDuplicates(rgs) + } + overrides[df.FileName] = rgs + } + + // 5. Overlay inline overrides onto the base model ------------------------------------ + owners := newOverlayOwners(baseCO, overrides) + + // 6. Validate aggregate required owners --------------------------------------------- + expectedAll := []string{ + "@base-test", + "@devops", + "@frontend-inline-misspellede", + "@model-owner", + "@qa-friend", + } + slices.Sort(expectedAll) + gotAll := owners.AllRequired().Flatten() + if !slices.Equal(gotAll, expectedAll) { + t.Fatalf("AllRequired mismatch. expected %v, got %v", expectedAll, gotAll) + } + + // 7. Validate per-file owners -------------------------------------------------------- + wantPerFile := map[string][]string{ + "frontend/a.test.ts": {"@qa-friend"}, + "frontend/b.ts": {"@frontend-inline-misspellede"}, + "models.py": {"@model-owner", "@devops"}, + "test_a.py": {"@base-test"}, + } + + for file, want := range wantPerFile { + gotRGs, ok := owners.FileRequired()[file] + if !ok { + t.Fatalf("expected file %s in owners map", file) + } + got := gotRGs.Flatten() + slices.Sort(want) + if !reflect.DeepEqual(got, want) { + t.Errorf("file %s: expected required %v, got %v", file, want, got) + } + } +} diff --git a/internal/app/overlay_test.go b/internal/app/overlay_test.go new file mode 100644 index 0000000..2d9a8bf --- /dev/null +++ b/internal/app/overlay_test.go @@ -0,0 +1,49 @@ +package app + +import ( + "testing" + + "github.com/multimediallc/codeowners-plus/pkg/codeowners" +) + +type simpleOwners struct { + required map[string]codeowners.ReviewerGroups +} + +func (s *simpleOwners) SetAuthor(a string) {} +func (s *simpleOwners) FileRequired() map[string]codeowners.ReviewerGroups { return s.required } +func (s *simpleOwners) FileOptional() map[string]codeowners.ReviewerGroups { return nil } +func (s *simpleOwners) AllRequired() codeowners.ReviewerGroups { + agg := codeowners.ReviewerGroups{} + for _, g := range s.required { + agg = append(agg, g...) + } + return agg +} +func (s *simpleOwners) AllOptional() codeowners.ReviewerGroups { return nil } +func (s *simpleOwners) UnownedFiles() []string { return nil } +func (s *simpleOwners) ApplyApprovals([]string) {} +func (s *simpleOwners) AddInlineOwners(string, codeowners.ReviewerGroups) {} + +func TestOverlayOwnersPrecedence(t *testing.T) { + rgm := codeowners.NewReviewerGroupMemo() + baseGrp := rgm.ToReviewerGroup("@file") + inGrp := rgm.ToReviewerGroup("@inline") + + base := &simpleOwners{required: map[string]codeowners.ReviewerGroups{ + "file.go": {baseGrp}, + }} + + overlay := newOverlayOwners(base, map[string]codeowners.ReviewerGroups{ + "file.go": {inGrp}, + }).(*overlayOwners) + + req := overlay.FileRequired() + if len(req) != (1) { + t.Fatalf("expected 1 entry") + } + grps := req["file.go"] + if len(grps) != 1 || grps[0] != inGrp { + t.Fatalf("precedence failed, got %+v", grps) + } +} diff --git a/internal/config/config.go b/internal/config/config.go index 85d0445..138bffb 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -9,12 +9,13 @@ import ( ) type Config struct { - MaxReviews *int `toml:"max_reviews"` - MinReviews *int `toml:"min_reviews"` - UnskippableReviewers []string `toml:"unskippable_reviewers"` - Ignore []string `toml:"ignore"` - Enforcement *Enforcement `toml:"enforcement"` - HighPriorityLabels []string `toml:"high_priority_labels"` + MaxReviews *int `toml:"max_reviews"` + MinReviews *int `toml:"min_reviews"` + UnskippableReviewers []string `toml:"unskippable_reviewers"` + Ignore []string `toml:"ignore"` + Enforcement *Enforcement `toml:"enforcement"` + HighPriorityLabels []string `toml:"high_priority_labels"` + InlineOwnershipEnabled bool `toml:"inline_ownership_enabled"` } type Enforcement struct { @@ -28,12 +29,13 @@ func ReadConfig(path string) (*Config, error) { } defaultConfig := &Config{ - MaxReviews: nil, - MinReviews: nil, - UnskippableReviewers: []string{}, - Ignore: []string{}, - Enforcement: &Enforcement{Approval: false, FailCheck: true}, - HighPriorityLabels: []string{}, + MaxReviews: nil, + MinReviews: nil, + UnskippableReviewers: []string{}, + Ignore: []string{}, + Enforcement: &Enforcement{Approval: false, FailCheck: true}, + HighPriorityLabels: []string{}, + InlineOwnershipEnabled: false, } fileName := path + "codeowners.toml" diff --git a/internal/config/config_test.go b/internal/config/config_test.go index d1622cf..72de7e6 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -179,6 +179,21 @@ func TestReadConfigFileError(t *testing.T) { } } +func TestReadConfig_InlineOwnershipEnabled(t *testing.T) { + tmpDir := t.TempDir() + content := `inline_ownership_enabled = true` + if err := os.WriteFile(filepath.Join(tmpDir, "codeowners.toml"), []byte(content), 0644); err != nil { + t.Fatalf("write file: %v", err) + } + cfg, err := ReadConfig(tmpDir) + if err != nil { + t.Fatalf("ReadConfig error: %v", err) + } + if !cfg.InlineOwnershipEnabled { + t.Errorf("expected InlineOwnershipEnabled true, got false") + } +} + // Helper functions func intPtr(i int) *int { return &i diff --git a/pkg/codeowners/add_inline_test.go b/pkg/codeowners/add_inline_test.go new file mode 100644 index 0000000..e3aeda0 --- /dev/null +++ b/pkg/codeowners/add_inline_test.go @@ -0,0 +1,55 @@ +package codeowners + +import "testing" + +func TestAddInlineOwners_MergeAndDedup(t *testing.T) { + rgm := NewReviewerGroupMemo() + rgA := rgm.ToReviewerGroup("@a") + rgB := rgm.ToReviewerGroup("@b") + + fo := newFileOwners() + fo.requiredReviewers = ReviewerGroups{rgA} + + om := &ownersMap{ + fileToOwner: map[string]fileOwners{"file.go": *fo}, + nameReviewerMap: map[string]ReviewerGroups{"@a": {rgA}}, + } + + // Add B and duplicate A + om.AddInlineOwners("file.go", ReviewerGroups{rgB, rgA}) + + fr := om.FileRequired() + groups := fr["file.go"] + if len(groups) != 2 { + t.Fatalf("expected 2 reviewer groups, got %d", len(groups)) + } + flattened := groups.Flatten() + if !(len(flattened) == 2 && flattened[0] == "@a" && flattened[1] == "@b") && !(flattened[0] == "@b" && flattened[1] == "@a") { + t.Errorf("unexpected names: %v", flattened) + } + + // Ensure reverse lookup updated + if _, ok := om.nameReviewerMap["@b"]; !ok { + t.Errorf("nameReviewerMap not updated for @b") + } +} + +func TestAddInlineOwners_FileNotPresent(t *testing.T) { + rgm := NewReviewerGroupMemo() + rgC := rgm.ToReviewerGroup("@c") + + om := &ownersMap{ + fileToOwner: make(map[string]fileOwners), + nameReviewerMap: make(map[string]ReviewerGroups), + } + + om.AddInlineOwners("new.go", ReviewerGroups{rgC}) + + fr := om.FileRequired() + if len(fr) != 1 { + t.Fatalf("expected 1 file entry, got %d", len(fr)) + } + if _, ok := fr["new.go"]; !ok { + t.Fatalf("new.go not in FileRequired map") + } +} diff --git a/pkg/codeowners/codeowners.go b/pkg/codeowners/codeowners.go index 070c943..d8938ea 100644 --- a/pkg/codeowners/codeowners.go +++ b/pkg/codeowners/codeowners.go @@ -3,9 +3,10 @@ package codeowners import ( "errors" "io" + "slices" "strings" - "github.com/multimediallc/codeowners-plus/pkg/functional" + f "github.com/multimediallc/codeowners-plus/pkg/functional" ) // CodeOwners represents a collection of owned files, with reverse lookups for owners and reviewers @@ -30,6 +31,10 @@ type CodeOwners interface { // ApplyApprovals marks the given approvers as satisfied ApplyApprovals(approvers []string) + + // AddInlineOwners merges additional required owners for a specific file, typically + // coming from inline ownership blocks. Owners already present are deduplicated. + AddInlineOwners(file string, owners ReviewerGroups) } // New creates a new CodeOwners object from a root path and a list of diff files @@ -116,6 +121,37 @@ func (om *ownersMap) ApplyApprovals(approvers []string) { } } +// AddInlineOwners appends the given reviewer groups to the required list for the +// specified file, deduplicating and updating reverse lookup maps. +// TODO|ZB should we rename this? is it actually specific to inline owners? +func (om *ownersMap) AddInlineOwners(file string, owners ReviewerGroups) { + fo, ok := om.fileToOwner[file] + if !ok { + foPtr := newFileOwners() + om.fileToOwner[file] = *foPtr + fo = om.fileToOwner[file] + } + // Merge while avoiding duplicates at pointer level. + existing := fo.requiredReviewers + for _, rg := range owners { + duplicate := false + for _, ex := range existing { + if slices.Equal(ex.Names, rg.Names) { + duplicate = true + break + } + } + if !duplicate { + fo.requiredReviewers = append(fo.requiredReviewers, rg) + existing = append(existing, rg) + for _, name := range rg.Names { + om.nameReviewerMap[name] = append(om.nameReviewerMap[name], rg) + } + } + } + om.fileToOwner[file] = fo +} + type ownerTreeNode struct { name string parent *ownerTreeNode diff --git a/pkg/inlineowners/oracle.go b/pkg/inlineowners/oracle.go new file mode 100644 index 0000000..e4d6b4d --- /dev/null +++ b/pkg/inlineowners/oracle.go @@ -0,0 +1,41 @@ +package inlineowners + +// Block represents an inline ownership region in a file. +// Start and End are inclusive line numbers (1-based). +// Empty-content blocks have StartLine > EndLine; they are ignored in range look-ups. +// Owners must already be trimmed/deduped. + +type Block struct { + Owners []string + Start int + End int +} + +// Oracle maps a file path (relative to repository root) to its inline ownership blocks. +// It can answer ownership queries for any line range. + +type Oracle map[string][]Block + +// OwnersForRange returns a slice of owner lists for every block that overlaps the +// given line interval [start,end] (inclusive). If no block overlaps, nil is returned. +// Empty-content blocks (Start > End) never match. +// The caller may deduplicate the returned owners if needed. +func (o Oracle) OwnersForRange(file string, start, end int) [][]string { + blocks, ok := o[file] + if !ok { + return nil + } + matches := [][]string{} + for _, b := range blocks { + if b.Start > b.End { // empty block, ignore + continue + } + if end >= b.Start && start <= b.End { + matches = append(matches, b.Owners) + } + } + if len(matches) == 0 { + return nil + } + return matches +} diff --git a/pkg/inlineowners/oracle_test.go b/pkg/inlineowners/oracle_test.go new file mode 100644 index 0000000..641ce49 --- /dev/null +++ b/pkg/inlineowners/oracle_test.go @@ -0,0 +1,36 @@ +package inlineowners + +import "testing" + +func makeOracle() Oracle { + return Oracle{ + "file.go": { + {Owners: []string{"a"}, Start: 10, End: 20}, + {Owners: []string{"b"}, Start: 30, End: 40}, + }, + } +} + +func TestOwnersForRange_FullInside(t *testing.T) { + o := makeOracle() + owners := o.OwnersForRange("file.go", 12, 18) + if len(owners) != 1 || owners[0][0] != "a" { + t.Fatalf("expected owner a, got %+v", owners) + } +} + +func TestOwnersForRange_PartialOverlap(t *testing.T) { + o := makeOracle() + owners := o.OwnersForRange("file.go", 25, 32) + if len(owners) != 1 || owners[0][0] != "b" { + t.Fatalf("expected owner b, got %+v", owners) + } +} + +func TestOwnersForRange_Outside(t *testing.T) { + o := makeOracle() + owners := o.OwnersForRange("file.go", 1, 5) + if owners != nil { + t.Fatalf("expected nil owners, got %+v", owners) + } +} diff --git a/pkg/inlineowners/parser.go b/pkg/inlineowners/parser.go new file mode 100644 index 0000000..6d43cdb --- /dev/null +++ b/pkg/inlineowners/parser.go @@ -0,0 +1,153 @@ +package inlineowners + +import ( + "bufio" + "fmt" + "io" + "regexp" + "strings" +) + +// InlineBlock represents an inline ownership block extracted from a source file. +// Owners contains the list parsed from the start tag. StartLine/EndLine are +// 1-based indices of the lines inside the block (exclusive of the tag lines). +// If EndLine < StartLine the block is effectively empty but still returned so +// the caller can decide how to handle it. +// +// Example of a block in a Go file: +// +// // +// code here +// // +// +// Lines containing the two "//" tags are NOT included in the range. +// The first code line has index StartLine and the last code line is EndLine. +// +// The parser is tolerant: malformed or unmatched tags generate warnings via +// the provided writer but do not fail the parse completely. +// +// Currently supported comment prefixes: "//" and "#". Both may be preceded +// by arbitrary whitespace. +// Tag names are matched case-insensitively. +// +// The parser also supports the edge-case where the start and end tag both +// appear on the same line (e.g. `// /* code */ // `). +// In such a scenario the block's content lines are considered empty; the +// returned InlineBlock will have StartLine > EndLine so that callers can treat +// it as a zero-length range. +type InlineBlock struct { + Owners []string + StartLine int + EndLine int +} + +var ( + startTagRe = regexp.MustCompile(`(?i)^\s*(?P//|#)\s*[^}]*)}>\s*$`) + endTagRe = regexp.MustCompile(`(?i)^\s*(?://|#)\s*\s*$`) + // start tag that can exist *anywhere* in a line (used when start & end + // tags share the same line; we don't anchor with ^ and $) + inlineStartAnywhereRe = regexp.MustCompile(`(?i)[^}]*)}>`) + endAnywhereRe = regexp.MustCompile(`(?i)`) // end tag anywhere in line +) + +// Parse scans the given file content and returns all inline ownership blocks. +// Any warnings (unclosed tags, malformed owner list, nested blocks, etc.) are +// written to warn. The function never returns an error; severe issues are also +// reported through warn so callers can decide whether to fail the build. +func Parse(content string, warn io.Writer) ([]InlineBlock, error) { + blocks := []InlineBlock{} + + scanner := bufio.NewScanner(strings.NewReader(content)) + lineNo := 0 + inBlock := false + var current InlineBlock + for scanner.Scan() { + lineNo++ + line := strings.TrimRight(scanner.Text(), "\r") + + lower := strings.ToLower(line) + + // Fast path: both start & end on same line + if openIdx := inlineStartAnywhereRe.FindStringIndex(line); openIdx != nil { + endIdx := endAnywhereRe.FindStringIndex(line[openIdx[1]:]) + if endIdx != nil { + ownersRaw := inlineStartAnywhereRe.FindStringSubmatch(line)[inlineStartAnywhereRe.SubexpIndex("owners")] + ownersSlice := splitOwners(strings.TrimSpace(ownersRaw)) + if len(ownersSlice) == 0 { + _, _ = fmt.Fprintf(warn, "WARNING: Empty owner list in inline tag at line %d\n", lineNo) + } + blk := InlineBlock{ + Owners: ownersSlice, + StartLine: lineNo + 1, // no interior lines + EndLine: lineNo, // StartLine > EndLine -> empty content + } + blocks = append(blocks, blk) + // Remove this occurence so outer logic doesn't treat it twice + continue + } + } + + if m := startTagRe.FindStringSubmatch(line); m != nil { + if inBlock { + // nested start tag – disallowed + _, _ = fmt.Fprintf(warn, "WARNING: Nested tag at line %d ignored\n", lineNo) + continue + } + inBlock = true + ownersRaw := strings.TrimSpace(m[startTagRe.SubexpIndex("owners")]) + ownersSlice := splitOwners(ownersRaw) + if len(ownersSlice) == 0 { + _, _ = fmt.Fprintf(warn, "WARNING: Empty owner list in inline tag at line %d\n", lineNo) + } + current = InlineBlock{ + Owners: ownersSlice, + StartLine: lineNo + 1, // first line AFTER the start tag + } + continue + } + + if endTagRe.MatchString(line) { + if !inBlock { + // stray end tag + _, _ = fmt.Fprintf(warn, "WARNING: Unmatched tag at line %d ignored\n", lineNo) + continue + } + // Finalise current block + current.EndLine = lineNo - 1 // last line BEFORE the end tag + blocks = append(blocks, current) + inBlock = false + continue + } + + // Detect malformed starting attempt e.g. missing braces or > + if strings.Contains(lower, " tag at line %d ignored\n", lineNo) + } + } + + if inBlock { + // EOF reached without closing tag + _, _ = fmt.Fprintf(warn, "WARNING: tag starting at line %d not closed before EOF\n", current.StartLine-1) + // Set EndLine to last line of file + current.EndLine = lineNo + blocks = append(blocks, current) + } + + return blocks, nil +} + +func splitOwners(raw string) []string { + if raw == "" { + return nil + } + parts := strings.Split(raw, ",") + owners := make([]string, 0, len(parts)) + for _, p := range parts { + trimmed := strings.TrimSpace(p) + if trimmed != "" { + owners = append(owners, trimmed) + } + } + return owners +} diff --git a/pkg/inlineowners/parser_test.go b/pkg/inlineowners/parser_test.go new file mode 100644 index 0000000..79aa362 --- /dev/null +++ b/pkg/inlineowners/parser_test.go @@ -0,0 +1,191 @@ +package inlineowners + +import ( + "bytes" + "testing" +) + +func TestParse_SingleBlock(t *testing.T) { + src := `// +line1 +line2 +// +` + warn := &bytes.Buffer{} + blocks, err := Parse(src, warn) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(blocks) != 1 { + t.Fatalf("expected 1 block, got %d", len(blocks)) + } + b := blocks[0] + if b.StartLine != 2 || b.EndLine != 3 { + t.Errorf("unexpected line range: got (%d,%d)", b.StartLine, b.EndLine) + } + expectedOwners := []string{"@alice", "@bob"} + if len(b.Owners) != len(expectedOwners) { + t.Fatalf("expected %d owners, got %d", len(expectedOwners), len(b.Owners)) + } + for i, o := range expectedOwners { + if b.Owners[i] != o { + t.Errorf("owner %d mismatch: expected %s, got %s", i, o, b.Owners[i]) + } + } + if warn.Len() != 0 { + t.Errorf("unexpected warnings: %s", warn.String()) + } +} + +func TestParse_MultipleBlocksWithHashComments(t *testing.T) { + src := `# +a +# +# +b +c +# +` + warn := &bytes.Buffer{} + blocks, _ := Parse(src, warn) + if len(blocks) != 2 { + t.Fatalf("expected 2 blocks, got %d", len(blocks)) + } + if blocks[0].StartLine != 2 || blocks[0].EndLine != 2 { + t.Errorf("first block line range wrong: %+v", blocks[0]) + } + if blocks[1].StartLine != 5 || blocks[1].EndLine != 6 { + t.Errorf("second block line range wrong: %+v", blocks[1]) + } +} + +func TestParse_UnclosedTag(t *testing.T) { + src := `// +code +` + warn := &bytes.Buffer{} + blocks, _ := Parse(src, warn) + if len(blocks) != 1 { + t.Fatalf("expected 1 block, got %d", len(blocks)) + } + if warn.Len() == 0 { + t.Errorf("expected warning for unclosed tag, got none") + } +} + +func TestParse_NestedTagWarning(t *testing.T) { + src := `// +// +code +// +// +` + warn := &bytes.Buffer{} + blocks, _ := Parse(src, warn) + if len(blocks) != 1 { + t.Fatalf("expected 1 block (inner ignored), got %d", len(blocks)) + } + if warn.Len() == 0 { + t.Errorf("expected warnings for nested tag, got none") + } +} + +func TestParse_SingleLineOpenClose(t *testing.T) { + src := ` // some code here // ` + warn := &bytes.Buffer{} + blocks, _ := Parse(src, warn) + if len(blocks) != 1 { + t.Fatalf("expected 1 block, got %d", len(blocks)) + } + blk := blocks[0] + if blk.StartLine <= blk.EndLine { + t.Errorf("expected empty content range, got Start=%d End=%d", blk.StartLine, blk.EndLine) + } + if len(blk.Owners) != 1 || blk.Owners[0] != "devs" { + t.Errorf("unexpected owners slice: %+v", blk.Owners) + } +} + +func TestParse_EsotericSpacingAndCase(t *testing.T) { + src := `# +# +` + warn := &bytes.Buffer{} + blocks, _ := Parse(src, warn) + if len(blocks) != 1 { + t.Fatalf("expected 1 block, got %d", len(blocks)) + } + blk := blocks[0] + if !(len(blk.Owners) == 2 && blk.Owners[0] == "team_one" && blk.Owners[1] == "team_two") { + t.Errorf("owners parsing failed: %+v", blk.Owners) + } +} + +func TestParse_EmptyOwnerList(t *testing.T) { + src := `// +// ` + warn := &bytes.Buffer{} + blocks, _ := Parse(src, warn) + if len(blocks) != 1 { + t.Fatalf("expected 1 block, got %d", len(blocks)) + } + if len(blocks[0].Owners) != 0 { + t.Errorf("expected empty owner list, got %+v", blocks[0].Owners) + } + if warn.Len() == 0 { + t.Errorf("expected warning for empty owner list, got none") + } +} + +func TestParse_StrayEndTag(t *testing.T) { + src := `// ` + warn := &bytes.Buffer{} + blocks, _ := Parse(src, warn) + if len(blocks) != 0 { + t.Fatalf("expected 0 blocks, got %d", len(blocks)) + } + if warn.Len() == 0 { + t.Errorf("expected warning for stray end tag") + } +} + +func TestParse_AdjacentBlocksNoBlankLine(t *testing.T) { + src := `// +// +// +code +// ` + warn := &bytes.Buffer{} + blocks, _ := Parse(src, warn) + if len(blocks) != 2 { + t.Fatalf("expected 2 blocks, got %d", len(blocks)) + } + if blocks[0].EndLine >= blocks[1].StartLine { + t.Errorf("blocks overlap unexpectedly: %+v %+v", blocks[0], blocks[1]) + } +} + +func TestParse_WindowsLineEndings(t *testing.T) { + src := "// \r\nline\r\n// \r\n" + warn := &bytes.Buffer{} + blocks, _ := Parse(src, warn) + if len(blocks) != 1 { + t.Fatalf("expected 1 block, got %d", len(blocks)) + } + if warn.Len() != 0 { + t.Errorf("unexpected warning: %s", warn.String()) + } +} + +func TestParse_MalformedStartTagIgnored(t *testing.T) { + src := `// ++ ++// ++console.log("looks like a new owner in this block"); ++// +diff --git a/test_project_inline/frontend/b.ts b/test_project_inline/frontend/b.ts +index d4c26b5..545b64d 100644 +--- a/test_project_inline/frontend/b.ts ++++ b/test_project_inline/frontend/b.ts +@@ -1,3 +1,3 @@ +-// ++// + console.log("bop"); + // +diff --git a/test_project_inline/models.py b/test_project_inline/models.py +index 8639e1b..7cea42e 100644 +--- a/test_project_inline/models.py ++++ b/test_project_inline/models.py +@@ -1,4 +1,5 @@ + # + world +-foo + # ++ ++ll +diff --git a/test_project_inline/test_a.py b/test_project_inline/test_a.py +:...skipping... +diff --git a/test_project_inline/frontend/a.test.ts b/test_project_inline/frontend/a.test.ts +index 7c61472..fbc730d 100644 +--- a/test_project_inline/frontend/a.test.ts ++++ b/test_project_inline/frontend/a.test.ts +@@ -5,3 +5,7 @@ console.log("quux"); + console.log("quuz"); + console.log("corge"); + // ++ ++// ++console.log("looks like a new owner in this block"); ++// +diff --git a/test_project_inline/frontend/b.ts b/test_project_inline/frontend/b.ts +index d4c26b5..545b64d 100644 +--- a/test_project_inline/frontend/b.ts ++++ b/test_project_inline/frontend/b.ts +@@ -1,3 +1,3 @@ +-// ++// + console.log("bop"); + // +diff --git a/test_project_inline/models.py b/test_project_inline/models.py +index 8639e1b..7cea42e 100644 +--- a/test_project_inline/models.py ++++ b/test_project_inline/models.py +@@ -1,4 +1,5 @@ + # + world +-foo + # ++ ++ll +diff --git a/test_project_inline/test_a.py b/test_project_inline/test_a.py +index 5716ca5..c4c70b0 100644 +:...skipping... +diff --git a/test_project_inline/frontend/a.test.ts b/test_project_inline/frontend/a.test.ts +index 7c61472..fbc730d 100644 +--- a/test_project_inline/frontend/a.test.ts ++++ b/test_project_inline/frontend/a.test.ts +@@ -5,3 +5,7 @@ console.log("quux"); + console.log("quuz"); + console.log("corge"); + // ++ ++// ++console.log("looks like a new owner in this block"); ++// +diff --git a/test_project_inline/frontend/b.ts b/test_project_inline/frontend/b.ts +index d4c26b5..545b64d 100644 +--- a/test_project_inline/frontend/b.ts ++++ b/test_project_inline/frontend/b.ts +@@ -1,3 +1,3 @@ +-// ++// + console.log("bop"); + // +diff --git a/test_project_inline/models.py b/test_project_inline/models.py +index 8639e1b..7cea42e 100644 +--- a/test_project_inline/models.py ++++ b/test_project_inline/models.py +@@ -1,4 +1,5 @@ + # + world +-foo + # ++ ++ll +diff --git a/test_project_inline/test_a.py b/test_project_inline/test_a.py +index 5716ca5..c4c70b0 100644 +--- a/test_project_inline/test_a.py ++++ b/test_project_inline/test_a.py +:...skipping... +diff --git a/test_project_inline/frontend/a.test.ts b/test_project_inline/frontend/a.test.ts +index 7c61472..fbc730d 100644 +--- a/test_project_inline/frontend/a.test.ts ++++ b/test_project_inline/frontend/a.test.ts +@@ -5,3 +5,7 @@ console.log("quux"); + console.log("quuz"); + console.log("corge"); + // ++ ++// ++console.log("looks like a new owner in this block"); ++// +diff --git a/test_project_inline/frontend/b.ts b/test_project_inline/frontend/b.ts +index d4c26b5..545b64d 100644 +--- a/test_project_inline/frontend/b.ts ++++ b/test_project_inline/frontend/b.ts +@@ -1,3 +1,3 @@ +-// ++// + console.log("bop"); + // +diff --git a/test_project_inline/models.py b/test_project_inline/models.py +index 8639e1b..7cea42e 100644 +--- a/test_project_inline/models.py ++++ b/test_project_inline/models.py +@@ -1,4 +1,5 @@ + # + world +-foo + # ++ ++ll +diff --git a/test_project_inline/test_a.py b/test_project_inline/test_a.py +index 5716ca5..c4c70b0 100644 +--- a/test_project_inline/test_a.py ++++ b/test_project_inline/test_a.py +@@ -1 +1 @@ +-bar +:...skipping... +diff --git a/test_project_inline/frontend/a.test.ts b/test_project_inline/frontend/a.test.ts +index 7c61472..fbc730d 100644 +--- a/test_project_inline/frontend/a.test.ts ++++ b/test_project_inline/frontend/a.test.ts +@@ -5,3 +5,7 @@ console.log("quux"); + console.log("quuz"); + console.log("corge"); + // ++ ++// ++console.log("looks like a new owner in this block"); ++// +diff --git a/test_project_inline/frontend/b.ts b/test_project_inline/frontend/b.ts +index d4c26b5..545b64d 100644 +--- a/test_project_inline/frontend/b.ts ++++ b/test_project_inline/frontend/b.ts +@@ -1,3 +1,3 @@ +-// ++// + console.log("bop"); + // +diff --git a/test_project_inline/models.py b/test_project_inline/models.py +index 8639e1b..7cea42e 100644 +--- a/test_project_inline/models.py ++++ b/test_project_inline/models.py +@@ -1,4 +1,5 @@ + # + world +-foo + # ++ ++ll +diff --git a/test_project_inline/test_a.py b/test_project_inline/test_a.py +index 5716ca5..c4c70b0 100644 +--- a/test_project_inline/test_a.py ++++ b/test_project_inline/test_a.py +@@ -1 +1 @@ +-bar ++bar d \ No newline at end of file diff --git a/test_project_inline/.diff_nochanges b/test_project_inline/.diff_nochanges new file mode 100644 index 0000000..e69de29 diff --git a/test_project_inline/a.py b/test_project_inline/a.py new file mode 100644 index 0000000..8e932ae --- /dev/null +++ b/test_project_inline/a.py @@ -0,0 +1,3 @@ +# +hello +# diff --git a/test_project_inline/b.py b/test_project_inline/b.py new file mode 100644 index 0000000..e69de29 diff --git a/test_project_inline/backend/test.txt b/test_project_inline/backend/test.txt new file mode 100644 index 0000000..bddaac1 --- /dev/null +++ b/test_project_inline/backend/test.txt @@ -0,0 +1,3 @@ +# +do oz +# diff --git a/test_project_inline/codeowners.toml b/test_project_inline/codeowners.toml new file mode 100644 index 0000000..c072b79 --- /dev/null +++ b/test_project_inline/codeowners.toml @@ -0,0 +1,12 @@ +unskippable_reviewers = ["@user1", "@user2"] + +# Test nil int value +# max_reviews = 2 + +ignore = ["ignored"] + +[enforcement] +approval = false +fail_check = true + +inline_ownership_enabled = true diff --git a/test_project_inline/empty/.gitkeep b/test_project_inline/empty/.gitkeep new file mode 100644 index 0000000..e69de29 diff --git a/test_project_inline/frontend/.codeowners b/test_project_inline/frontend/.codeowners new file mode 100644 index 0000000..b24b8ec --- /dev/null +++ b/test_project_inline/frontend/.codeowners @@ -0,0 +1,4 @@ +* @frontend +b.ts @b-owner +**/*test* @frontend-test +& **/*test* @frontend-core diff --git a/test_project_inline/frontend/a.test.ts b/test_project_inline/frontend/a.test.ts new file mode 100644 index 0000000..fbc730d --- /dev/null +++ b/test_project_inline/frontend/a.test.ts @@ -0,0 +1,11 @@ +// +// test lines +console.log("qux"); +console.log("quux"); +console.log("quuz"); +console.log("corge"); +// + +// +console.log("looks like a new owner in this block"); +// diff --git a/test_project_inline/frontend/a.ts b/test_project_inline/frontend/a.ts new file mode 100644 index 0000000..29d8e03 --- /dev/null +++ b/test_project_inline/frontend/a.ts @@ -0,0 +1,3 @@ +// +console.log("baz"); +// diff --git a/test_project_inline/frontend/b.ts b/test_project_inline/frontend/b.ts new file mode 100644 index 0000000..545b64d --- /dev/null +++ b/test_project_inline/frontend/b.ts @@ -0,0 +1,3 @@ +// +console.log("bop"); +// diff --git a/test_project_inline/frontend/inner/.codeowners b/test_project_inline/frontend/inner/.codeowners new file mode 100644 index 0000000..5652e8c --- /dev/null +++ b/test_project_inline/frontend/inner/.codeowners @@ -0,0 +1 @@ +a.js @inner-owner diff --git a/test_project_inline/frontend/inner/a.js b/test_project_inline/frontend/inner/a.js new file mode 100644 index 0000000..1a81792 --- /dev/null +++ b/test_project_inline/frontend/inner/a.js @@ -0,0 +1,3 @@ +// +console.log("cuh"); +// diff --git a/test_project_inline/frontend/inner/a.test.js b/test_project_inline/frontend/inner/a.test.js new file mode 100644 index 0000000..2370c83 --- /dev/null +++ b/test_project_inline/frontend/inner/a.test.js @@ -0,0 +1,3 @@ +// +console.log("duh"); +// \ No newline at end of file diff --git a/test_project_inline/frontend/inner/b.test.js b/test_project_inline/frontend/inner/b.test.js new file mode 100644 index 0000000..c08bc28 --- /dev/null +++ b/test_project_inline/frontend/inner/b.test.js @@ -0,0 +1,3 @@ +// +console.log("duh"); +// diff --git a/test_project_inline/frontend/inner/b.ts b/test_project_inline/frontend/inner/b.ts new file mode 100644 index 0000000..b98f012 --- /dev/null +++ b/test_project_inline/frontend/inner/b.ts @@ -0,0 +1,3 @@ +// +console.log("cip"); +// diff --git a/test_project_inline/models.py b/test_project_inline/models.py new file mode 100644 index 0000000..7cea42e --- /dev/null +++ b/test_project_inline/models.py @@ -0,0 +1,5 @@ +# +world +# + +ll diff --git a/test_project_inline/test_a.py b/test_project_inline/test_a.py new file mode 100644 index 0000000..c4c70b0 --- /dev/null +++ b/test_project_inline/test_a.py @@ -0,0 +1 @@ +bar d diff --git a/tools/cli/main_test.go b/tools/cli/main_test.go index cdb9731..8ea03bb 100644 --- a/tools/cli/main_test.go +++ b/tools/cli/main_test.go @@ -415,11 +415,12 @@ func (f *fakeCodeOwners) FileRequired() map[string]codeowners.ReviewerGroups { func (f *fakeCodeOwners) FileOptional() map[string]codeowners.ReviewerGroups { return f.optional } -func (f *fakeCodeOwners) SetAuthor(author string) {} -func (f *fakeCodeOwners) AllRequired() codeowners.ReviewerGroups { return nil } -func (f *fakeCodeOwners) AllOptional() codeowners.ReviewerGroups { return nil } -func (f *fakeCodeOwners) UnownedFiles() []string { return nil } -func (f *fakeCodeOwners) ApplyApprovals(approvers []string) {} +func (f *fakeCodeOwners) SetAuthor(author string) {} +func (f *fakeCodeOwners) AllRequired() codeowners.ReviewerGroups { return nil } +func (f *fakeCodeOwners) AllOptional() codeowners.ReviewerGroups { return nil } +func (f *fakeCodeOwners) UnownedFiles() []string { return nil } +func (f *fakeCodeOwners) ApplyApprovals(approvers []string) {} +func (f *fakeCodeOwners) AddInlineOwners(file string, owners codeowners.ReviewerGroups) {} func TestJsonTargets(t *testing.T) { owners := &fakeCodeOwners{