diff --git a/README.md b/README.md index 90857ad..add0ab0 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ Code Ownership & Review Assignment Tool - GitHub CODEOWNERS but better [![Go Report Card](https://goreportcard.com/badge/github.com/multimediallc/codeowners-plus)](https://goreportcard.com/report/github.com/multimediallc/codeowners-plus?kill_cache=1) [![Tests](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml/badge.svg)](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml) -![Coverage](https://img.shields.io/badge/Coverage-82.6%25-brightgreen) +![Coverage](https://img.shields.io/badge/Coverage-82.8%25-brightgreen) [![License](https://img.shields.io/badge/License-BSD%203--Clause-blue.svg)](https://opensource.org/licenses/BSD-3-Clause) [![Contributor Covenant](https://img.shields.io/badge/Contributor%20Covenant-2.1-4baaaa.svg)](CODE_OF_CONDUCT.md) @@ -22,6 +22,7 @@ Code Ownership & Review Assignment Tool - GitHub CODEOWNERS but better - [Advanced Configuration](#advanced-configuration) - [Enforcement Options](#enforcement-options) - [Quiet Mode](#quiet-mode) + - [Ownership Oracles](#ownership-oracles) - [CLI Tool](#cli-tool) - [Contributing](#contributing) - [Future Features](#future-features) @@ -48,6 +49,7 @@ These are features missing from GitHub code owners that are supported by Codeown * GitHub CODEOWNERS supports only `OR` ownership rules, in contrast * Directory-level code ownership files to assign fine-grained code ownership * Supports optional reviewers (cc users/teams for non-blocking reviews) +* Ownership oracles: external tooling can compute additional reviewer requirements from PR content (see [Ownership Oracles](#ownership-oracles)) * Advanced global configuration (see [Advanced Configuration](#advanced-configuration)) ## Getting Started @@ -372,6 +374,54 @@ Using the `quiet` input on the action will change the behavior in a couple ways: * **Draft Pull Requests:** This is a common use case. You might want the Codeowners Plus logic to run and report a status (e.g., pending or failed) on draft PRs, but without notifying reviewers prematurely by adding comments or requesting reviews until the PR is marked "Ready for review". * **Custom Notification Workflows:** You might prefer to handle notifications or review requests through a different mechanism and only use Codeowners Plus for the status check enforcement. +### Ownership Oracles + +Some ownership requirements cannot be expressed as path patterns: "changes to telemetry events need data-platform review" depends on what changed inside a file, not which file changed. Ownership oracles let external tooling compute these requirements and feed them to Codeowners Plus as data. + +An oracle file is JSON, written by any earlier workflow step: + +```json +{ + "rules": [ + { + "files": ["src/telemetry/**"], + "owners": ["@your-org/data-platform"], + "optional": false, + "reason": "telemetry event schema changed in this PR" + } + ] +} +``` + +* `files`: doublestar glob patterns matched against the full repo-relative paths of files changed in the PR (no leading slash; patterns are already anchored to the repo root, and a leading slash is rejected) +* `owners`: a single OR group where any one of the listed owners satisfies the rule (same semantics as a `.codeowners` line) +* `optional`: when `true`, owners are CC'd instead of required +* `reason`: human-readable explanation, shown in verbose output + +Pass oracle files to the action with the `oracle-files` input (comma-separated): + +```yaml + - name: 'Detect telemetry changes' + id: detect + run: ./scripts/detect-telemetry-changes.sh > /tmp/telemetry-oracle.json + + - name: 'Codeowners Plus' + uses: multimediallc/codeowners-plus@v1 + with: + github-token: '${{ secrets.GITHUB_TOKEN }}' + pr: '${{ github.event.pull_request.number }}' + oracle-files: '/tmp/telemetry-oracle.json' +``` + +Oracle requirements are AND-merged with `.codeowners` requirements, the same mechanism as `require_both_branch_reviewers`: review requesting, approval tracking, smart dismissal, and the status check all apply to oracle-derived owners the same as file-derived owners. + +Notes: + +* Oracle rules can only add reviewer requirements, never remove or weaken requirements from `.codeowners` files, so a tampered oracle file can at worst request extra reviews. +* The script that *generates* the oracle file is a different story: in a `pull_request`-triggered workflow it runs from the PR checkout, so a PR author could modify it in the same PR to emit no rules. If an oracle enforces security-critical reviews, protect the generator script with a `.codeowners` rule, or run it from the base branch (for example via a `pull_request_target` workflow). +* A missing or malformed oracle file is a hard error (the check fails), since silently skipping one would drop required reviews. +* A file matched by a required (non-`optional`) oracle rule counts as owned, so it is not reported as an unowned file. Optional rules CC reviewers but do not confer ownership. + ## CLI Tool A CLI tool is available which provides some utilities for working with `.codeowners` files. diff --git a/action.yml b/action.yml index bc63b95..43335b8 100644 --- a/action.yml +++ b/action.yml @@ -15,6 +15,10 @@ inputs: description: 'The owner and repository name. For example `octocat/Hello-World`' required: true default: '${{ github.repository }}' + oracle-files: + description: 'Comma-separated list of ownership oracle JSON files to AND-merge into .codeowners requirements' + required: false + default: '' verbose: description: 'Print debug info' required: false @@ -111,6 +115,7 @@ runs: INPUT_GITHUB-TOKEN: ${{ inputs.github-token }} INPUT_PR: ${{ inputs.pr }} INPUT_REPOSITORY: ${{ inputs.repository }} + INPUT_ORACLE-FILES: ${{ inputs.oracle-files }} INPUT_VERBOSE: ${{ inputs.verbose }} INPUT_QUIET: ${{ inputs.quiet }} BIN: ${{ steps.resolve.outputs.bin }} diff --git a/internal/app/app.go b/internal/app/app.go index d962024..abeb110 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -12,6 +12,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/oracle" ) // OutputData holds the data that will be written to GITHUB_OUTPUT @@ -55,6 +56,7 @@ type Config struct { RepoDir string PR int Repo string + OracleFiles []string Verbose bool Quiet bool InfoBuffer io.Writer @@ -166,6 +168,10 @@ func (a *App) Run() (*OutputData, error) { return &OutputData{}, fmt.Errorf("NewCodeOwners Error: %v", err) } } + codeOwners, err = a.applyOracles(codeOwners, gitDiff) + if err != nil { + return &OutputData{}, err + } a.codeowners = codeOwners // Initialize user reviewer map @@ -212,6 +218,34 @@ func (a *App) Run() (*OutputData, error) { return outputData, nil } +// applyOracles AND-merges ownership from the configured oracle files; a missing or malformed file is a hard error. +func (a *App) applyOracles(codeOwners codeowners.CodeOwners, gitDiff git.Diff) (codeowners.CodeOwners, error) { + if len(a.config.OracleFiles) == 0 { + return codeOwners, nil + } + + merged := &oracle.RuleSet{} + for _, path := range a.config.OracleFiles { + ruleSet, err := oracle.Load(path) + if err != nil { + return nil, fmt.Errorf("oracle error: %v", err) + } + merged.Rules = append(merged.Rules, ruleSet.Rules...) + } + if len(merged.Rules) == 0 { + a.printDebug("Oracle files contain no rules\n") + return codeOwners, nil + } + + for _, rule := range merged.Rules { + a.printDebug("Oracle rule: files=%v owners=%v optional=%t reason=%q\n", rule.Files, rule.Owners, rule.Optional, rule.Reason) + } + + changedFiles := f.Map(gitDiff.AllChanges(), func(file codeowners.DiffFile) string { return file.FileName }) + oracleOwners := merged.ToCodeOwners(changedFiles, a.config.WarningBuffer) + return codeowners.MergeCodeOwners(codeOwners, oracleOwners), nil +} + func (a *App) processApprovalsAndReviewers() (bool, string, []string, error) { message := "" diff --git a/internal/app/oracle_test.go b/internal/app/oracle_test.go new file mode 100644 index 0000000..094b9da --- /dev/null +++ b/internal/app/oracle_test.go @@ -0,0 +1,229 @@ +package app + +import ( + "bytes" + "os" + "path/filepath" + "slices" + "strings" + "testing" + + "github.com/multimediallc/codeowners-plus/pkg/codeowners" +) + +func TestNewFromFileOwners(t *testing.T) { + rgm := codeowners.NewReviewerGroupMemo() + co := codeowners.NewFromFileOwners( + map[string]codeowners.ReviewerGroups{ + "both.go": {rgm.ToReviewerGroup("@required")}, + "required.go": {rgm.ToReviewerGroup("@required"), rgm.ToReviewerGroup("@required")}, + }, + map[string]codeowners.ReviewerGroups{ + "both.go": {rgm.ToReviewerGroup("@optional")}, + "optional.go": {rgm.ToReviewerGroup("@optional")}, + }, + ) + + if len(co.FileRequired()["both.go"]) != 1 || len(co.FileOptional()["both.go"]) != 1 { + t.Errorf("expected both.go to carry one required and one optional group, got %+v / %+v", + co.FileRequired()["both.go"], co.FileOptional()["both.go"]) + } + if len(co.FileRequired()["required.go"]) != 1 { + t.Errorf("expected duplicate groups to be deduplicated, got %d", len(co.FileRequired()["required.go"])) + } + if _, ok := co.FileRequired()["optional.go"]; ok { + t.Error("optional-only file should have no required reviewers") + } + if len(co.UnownedFiles()) != 0 { + t.Errorf("expected no unowned files, got %v", co.UnownedFiles()) + } +} + +func writeOracleFile(t *testing.T, content string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "oracle.json") + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + return path +} + +func oracleTestApp(oracleFiles []string) *App { + return &App{ + config: &Config{ + OracleFiles: oracleFiles, + InfoBuffer: &bytes.Buffer{}, + WarningBuffer: &bytes.Buffer{}, + }, + } +} + +func baseCodeOwnersForOracleTest() codeowners.CodeOwners { + rgm := codeowners.NewReviewerGroupMemo() + return codeowners.NewFromFileOwners(map[string]codeowners.ReviewerGroups{ + "src/telemetry/events.ts": {rgm.ToReviewerGroup("@org/frontend")}, + }, nil) +} + +func TestApplyOraclesNoFiles(t *testing.T) { + app := oracleTestApp(nil) + base := baseCodeOwnersForOracleTest() + result, err := app.applyOracles(base, mockGitDiff{changes: []string{"src/telemetry/events.ts"}}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result != base { + t.Error("expected base CodeOwners to be returned unchanged when no oracle files are configured") + } +} + +func TestApplyOraclesMergesRequirements(t *testing.T) { + path := writeOracleFile(t, `{"rules": [ + {"files": ["src/telemetry/**"], "owners": ["@org/data-platform"], "reason": "NR event change"} + ]}`) + app := oracleTestApp([]string{path}) + base := baseCodeOwnersForOracleTest() + + result, err := app.applyOracles(base, mockGitDiff{changes: []string{"src/telemetry/events.ts", "other.go"}}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + groups := result.FileRequired()["src/telemetry/events.ts"] + names := codeowners.OriginalStrings(groups.Flatten()) + if !slices.Contains(names, "@org/frontend") || !slices.Contains(names, "@org/data-platform") { + t.Errorf("expected both base and oracle owners, got %v", names) + } + if len(groups) != 2 { + t.Errorf("expected 2 AND groups, got %d", len(groups)) + } +} + +func TestApplyOraclesMissingFile(t *testing.T) { + app := oracleTestApp([]string{"/nonexistent/oracle.json"}) + _, err := app.applyOracles(baseCodeOwnersForOracleTest(), mockGitDiff{changes: []string{"a.go"}}) + if err == nil || !strings.Contains(err.Error(), "oracle error") { + t.Errorf("expected hard error for missing oracle file, got %v", err) + } +} + +func TestApplyOraclesMultipleFiles(t *testing.T) { + pathA := writeOracleFile(t, `{"rules": [ + {"files": ["src/telemetry/**"], "owners": ["@org/data-platform"]} + ]}`) + pathB := writeOracleFile(t, `{"rules": [ + {"files": ["src/**"], "owners": ["@org/security"]} + ]}`) + app := oracleTestApp([]string{pathA, pathB}) + base := baseCodeOwnersForOracleTest() + + result, err := app.applyOracles(base, mockGitDiff{changes: []string{"src/telemetry/events.ts"}}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + groups := result.FileRequired()["src/telemetry/events.ts"] + names := codeowners.OriginalStrings(groups.Flatten()) + for _, expected := range []string{"@org/frontend", "@org/data-platform", "@org/security"} { + if !slices.Contains(names, expected) { + t.Errorf("expected owner %s from merged oracle files, got %v", expected, names) + } + } + if len(groups) != 3 { + t.Errorf("expected 3 AND groups (base + one per oracle file), got %d", len(groups)) + } +} + +func TestApplyOraclesUnownedFileBecomesOwned(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "a.go"), []byte("package a"), 0o644); err != nil { + t.Fatal(err) + } + diff := mockGitDiff{changes: []string{"a.go"}} + base, err := codeowners.New(dir, diff.AllChanges(), nil, &bytes.Buffer{}) + if err != nil { + t.Fatal(err) + } + if len(base.UnownedFiles()) != 1 { + t.Fatalf("expected a.go to start unowned, got %v", base.UnownedFiles()) + } + + path := writeOracleFile(t, `{"rules": [{"files": ["a.go"], "owners": ["@org/adopters"]}]}`) + app := oracleTestApp([]string{path}) + result, err := app.applyOracles(base, diff) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(result.UnownedFiles()) != 0 { + t.Errorf("oracle-matched file should count as owned, got unowned: %v", result.UnownedFiles()) + } +} + +func TestApplyOraclesUnmatchedFileStaysUnowned(t *testing.T) { + dir := t.TempDir() + for _, name := range []string{"a.go", "b.go"} { + if err := os.WriteFile(filepath.Join(dir, name), []byte("package x"), 0o644); err != nil { + t.Fatal(err) + } + } + diff := mockGitDiff{changes: []string{"a.go", "b.go"}} + base, err := codeowners.New(dir, diff.AllChanges(), nil, &bytes.Buffer{}) + if err != nil { + t.Fatal(err) + } + if len(base.UnownedFiles()) != 2 { + t.Fatalf("expected both files to start unowned, got %v", base.UnownedFiles()) + } + + path := writeOracleFile(t, `{"rules": [{"files": ["a.go"], "owners": ["@org/adopters"]}]}`) + app := oracleTestApp([]string{path}) + result, err := app.applyOracles(base, diff) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if unowned := result.UnownedFiles(); !slices.Equal(unowned, []string{"b.go"}) { + t.Errorf("expected only b.go to remain unowned, got %v", unowned) + } +} + +func TestApplyOraclesOptionalRuleKeepsUnowned(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "a.go"), []byte("package a"), 0o644); err != nil { + t.Fatal(err) + } + diff := mockGitDiff{changes: []string{"a.go"}} + base, err := codeowners.New(dir, diff.AllChanges(), nil, &bytes.Buffer{}) + if err != nil { + t.Fatal(err) + } + if len(base.UnownedFiles()) != 1 { + t.Fatalf("expected a.go to start unowned, got %v", base.UnownedFiles()) + } + + path := writeOracleFile(t, `{"rules": [{"files": ["a.go"], "owners": ["@org/watchers"], "optional": true}]}`) + app := oracleTestApp([]string{path}) + result, err := app.applyOracles(base, diff) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if unowned := result.UnownedFiles(); !slices.Equal(unowned, []string{"a.go"}) { + t.Errorf("optional-only oracle match should not confer ownership, got unowned: %v", unowned) + } + optional := result.FileOptional()["a.go"] + if len(optional) != 1 { + t.Errorf("expected the optional group to still be attached, got %v", optional) + } +} + +func TestApplyOraclesEmptyRules(t *testing.T) { + path := writeOracleFile(t, `{"rules": []}`) + app := oracleTestApp([]string{path}) + base := baseCodeOwnersForOracleTest() + result, err := app.applyOracles(base, mockGitDiff{changes: []string{"a.go"}}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result != base { + t.Error("expected base CodeOwners to be returned unchanged for empty oracle rules") + } +} diff --git a/main.go b/main.go index ecd3d9f..47ca223 100644 --- a/main.go +++ b/main.go @@ -8,6 +8,7 @@ import ( "io" "os" "strconv" + "strings" "testing" "github.com/multimediallc/codeowners-plus/internal/app" @@ -15,22 +16,24 @@ import ( // Flags holds the command line flags type Flags struct { - Token *string - RepoDir *string - PR *int - Repo *string - Verbose *bool - Quiet *bool + Token *string + RepoDir *string + PR *int + Repo *string + OracleFiles *string + Verbose *bool + Quiet *bool } var ( flags = &Flags{ - Token: flag.String("token", getEnv("INPUT_GITHUB-TOKEN", ""), "GitHub authentication token"), - RepoDir: flag.String("dir", getEnv("GITHUB_WORKSPACE", "/"), "Path to local Git repo"), - PR: flag.Int("pr", ignoreError(strconv.Atoi(getEnv("INPUT_PR", ""))), "Pull Request number"), - Repo: flag.String("repo", getEnv("INPUT_REPOSITORY", ""), "GitHub repo name"), - Verbose: flag.Bool("v", ignoreError(strconv.ParseBool(getEnv("INPUT_VERBOSE", "0"))), "Verbose output"), - Quiet: flag.Bool("quiet", ignoreError(strconv.ParseBool(getEnv("INPUT_QUIET", "0"))), "Disable PR comments and review requests"), + Token: flag.String("token", getEnv("INPUT_GITHUB-TOKEN", ""), "GitHub authentication token"), + RepoDir: flag.String("dir", getEnv("GITHUB_WORKSPACE", "/"), "Path to local Git repo"), + PR: flag.Int("pr", ignoreError(strconv.Atoi(getEnv("INPUT_PR", ""))), "Pull Request number"), + Repo: flag.String("repo", getEnv("INPUT_REPOSITORY", ""), "GitHub repo name"), + OracleFiles: flag.String("oracle-files", getEnv("INPUT_ORACLE-FILES", ""), "Comma-separated list of ownership oracle JSON files"), + Verbose: flag.Bool("v", ignoreError(strconv.ParseBool(getEnv("INPUT_VERBOSE", "0"))), "Verbose output"), + Quiet: flag.Bool("quiet", ignoreError(strconv.ParseBool(getEnv("INPUT_QUIET", "0"))), "Disable PR comments and review requests"), } WarningBuffer = bytes.NewBuffer([]byte{}) InfoBuffer = bytes.NewBuffer([]byte{}) @@ -73,6 +76,17 @@ func ignoreError[V any, E error](res V, _ E) V { return res } +func splitOracleFiles(value string) []string { + parts := strings.Split(value, ",") + files := make([]string, 0, len(parts)) + for _, part := range parts { + if trimmed := strings.TrimSpace(part); trimmed != "" { + files = append(files, trimmed) + } + } + return files +} + func outputAndExit(w io.Writer, shouldFail bool, message string) { _, err := WarningBuffer.WriteTo(w) if err != nil { @@ -130,6 +144,7 @@ func main() { RepoDir: *flags.RepoDir, PR: *flags.PR, Repo: *flags.Repo, + OracleFiles: splitOracleFiles(*flags.OracleFiles), Verbose: *flags.Verbose, Quiet: *flags.Quiet, InfoBuffer: InfoBuffer, diff --git a/main_test.go b/main_test.go index df0f29c..9264fd3 100644 --- a/main_test.go +++ b/main_test.go @@ -14,11 +14,12 @@ import ( func init() { // Initialize test flags with default values flags = &Flags{ - Token: new(string), - RepoDir: new(string), - PR: new(int), - Repo: new(string), - Verbose: new(bool), + Token: new(string), + RepoDir: new(string), + PR: new(int), + Repo: new(string), + OracleFiles: new(string), + Verbose: new(bool), } *flags.Token = "test-token" *flags.RepoDir = "/test/dir" @@ -101,6 +102,33 @@ func TestIgnoreError(t *testing.T) { } } +func TestSplitOracleFiles(t *testing.T) { + tt := []struct { + name string + input string + expected []string + }{ + {name: "empty input", input: "", expected: []string{}}, + {name: "single file", input: "a.json", expected: []string{"a.json"}}, + {name: "multiple files", input: "a.json,b.json", expected: []string{"a.json", "b.json"}}, + {name: "whitespace trimmed", input: " a.json , b.json ", expected: []string{"a.json", "b.json"}}, + {name: "empty entries dropped", input: ",a.json,,", expected: []string{"a.json"}}, + } + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + result := splitOracleFiles(tc.input) + if len(result) != len(tc.expected) { + t.Fatalf("expected %v, got %v", tc.expected, result) + } + for i := range result { + if result[i] != tc.expected[i] { + t.Errorf("expected %v, got %v", tc.expected, result) + } + } + }) + } +} + func TestInitFlags(t *testing.T) { tokenStr := "test-token" prInt := 123 diff --git a/pkg/codeowners/codeowners.go b/pkg/codeowners/codeowners.go index afdddb0..6fab76f 100644 --- a/pkg/codeowners/codeowners.go +++ b/pkg/codeowners/codeowners.go @@ -55,6 +55,30 @@ func New(root string, files []DiffFile, fileReader FileReader, warningWriter io. return ownersMap, err } +// NewFromFileOwners creates a CodeOwners from explicit file-to-reviewers maps; files absent from both maps are untracked, not unowned. +func NewFromFileOwners(required map[string]ReviewerGroups, optional map[string]ReviewerGroups) CodeOwners { + fileToOwner := make(map[string]fileOwners) + for file, groups := range required { + fileToOwner[file] = fileOwners{ + requiredReviewers: f.RemoveDuplicates(groups), + optionalReviewers: make(ReviewerGroups, 0), + } + } + for file, groups := range optional { + owners, ok := fileToOwner[file] + if !ok { + owners = *newFileOwners() + } + owners.optionalReviewers = f.RemoveDuplicates(groups) + fileToOwner[file] = owners + } + return &ownersMap{ + fileToOwner: fileToOwner, + nameReviewerMap: buildNameReviewerMap(fileToOwner), + unownedFiles: []string{}, + } +} + // A collection of owned files, with reverse lookups for owners and reviewers type ownersMap struct { author string diff --git a/pkg/codeowners/merger.go b/pkg/codeowners/merger.go index a63542c..abf34f5 100644 --- a/pkg/codeowners/merger.go +++ b/pkg/codeowners/merger.go @@ -40,8 +40,14 @@ func MergeCodeOwners(base CodeOwners, head CodeOwners) CodeOwners { } } - // Merge unowned files - mergedUnowned := mergeUnownedFiles(base.UnownedFiles(), head.UnownedFiles(), allFiles) + // Only required owners confer ownership; optional-only files stay unowned. + filesWithRequiredOwners := make([]string, 0, len(mergedFileToOwner)) + for file, owners := range mergedFileToOwner { + if len(owners.requiredReviewers) > 0 { + filesWithRequiredOwners = append(filesWithRequiredOwners, file) + } + } + mergedUnowned := mergeUnownedFiles(base.UnownedFiles(), head.UnownedFiles(), filesWithRequiredOwners) // Build nameReviewerMap for approval tracking nameReviewerMap := buildNameReviewerMap(mergedFileToOwner) diff --git a/pkg/codeowners/merger_test.go b/pkg/codeowners/merger_test.go index 43a68a1..da62d8c 100644 --- a/pkg/codeowners/merger_test.go +++ b/pkg/codeowners/merger_test.go @@ -178,6 +178,24 @@ func TestMergeCodeOwners(t *testing.T) { expectedOptional: map[string][]string{}, expectedUnowned: []string{}, // Should not be in unowned since it has owners }, + { + name: "optional-only owners do not confer ownership", + baseRequired: map[string]ReviewerGroups{}, + headRequired: map[string]ReviewerGroups{}, + baseOptional: map[string]ReviewerGroups{}, + headOptional: map[string]ReviewerGroups{ + "file.py": { + {Names: NewSlugs([]string{"@watcher"}), Approved: false}, + }, + }, + baseUnowned: []string{"file.py"}, + headUnowned: []string{}, + expectedRequired: map[string][]string{}, + expectedOptional: map[string][]string{ + "file.py": {"@watcher"}, + }, + expectedUnowned: []string{"file.py"}, + }, { name: "OR groups within each branch", baseRequired: map[string]ReviewerGroups{ diff --git a/pkg/oracle/oracle.go b/pkg/oracle/oracle.go new file mode 100644 index 0000000..76fc585 --- /dev/null +++ b/pkg/oracle/oracle.go @@ -0,0 +1,114 @@ +// Package oracle loads externally computed reviewer requirements (see "Ownership Oracles" in the README). +package oracle + +import ( + "encoding/json" + "fmt" + "io" + "os" + "strings" + + "github.com/bmatcuk/doublestar/v4" + "github.com/multimediallc/codeowners-plus/pkg/codeowners" +) + +// Rule is a single oracle reviewer requirement. +type Rule struct { + // Files are doublestar globs matched against repo-relative changed paths. + Files []string `json:"files"` + // Owners is an OR group: any one listed reviewer satisfies the rule. + Owners []string `json:"owners"` + // Optional owners are CC'd instead of required. + Optional bool `json:"optional"` + // Reason is surfaced in verbose output. + Reason string `json:"reason"` +} + +// RuleSet is the parsed contents of an oracle file. +type RuleSet struct { + Rules []Rule `json:"rules"` +} + +// Parse decodes oracle JSON; rules that cannot take effect are errors, not skipped. +func Parse(data []byte) (*RuleSet, error) { + var ruleSet RuleSet + if err := json.Unmarshal(data, &ruleSet); err != nil { + return nil, fmt.Errorf("invalid oracle JSON: %w", err) + } + for i, rule := range ruleSet.Rules { + if len(rule.Files) == 0 { + return nil, fmt.Errorf("oracle rule %d has no files", i) + } + for _, pattern := range rule.Files { + // Empty patterns are "valid" per doublestar but match nothing. + if pattern == "" || !doublestar.ValidatePattern(pattern) { + return nil, fmt.Errorf("oracle rule %d has an invalid pattern %q", i, pattern) + } + // Doublestar globs are root-anchored; a leading slash never matches. + if strings.HasPrefix(pattern, "/") { + return nil, fmt.Errorf("oracle rule %d has a leading-slash pattern %q (patterns are repo-root-relative; drop the leading slash)", i, pattern) + } + } + if len(rule.Owners) == 0 { + return nil, fmt.Errorf("oracle rule %d has no owners", i) + } + for _, owner := range rule.Owners { + if strings.TrimSpace(owner) == "" { + return nil, fmt.Errorf("oracle rule %d has an empty owner", i) + } + } + } + return &ruleSet, nil +} + +// Load reads and parses an oracle file. +func Load(path string) (*RuleSet, error) { + data, err := os.ReadFile(path) + if err != nil { + return nil, fmt.Errorf("reading oracle file: %w", err) + } + ruleSet, err := Parse(data) + if err != nil { + return nil, fmt.Errorf("oracle file %s: %w", path, err) + } + return ruleSet, nil +} + +// ToCodeOwners builds the rules' requirements for changedFiles, for AND-merging via codeowners.MergeCodeOwners. +func (rs *RuleSet) ToCodeOwners(changedFiles []string, warningWriter io.Writer) codeowners.CodeOwners { + if warningWriter == nil { + warningWriter = io.Discard + } + rgm := codeowners.NewReviewerGroupMemo() + required := make(map[string]codeowners.ReviewerGroups) + optional := make(map[string]codeowners.ReviewerGroups) + for _, rule := range rs.Rules { + group := rgm.ToReviewerGroup(rule.Owners...) + target := required + if rule.Optional { + target = optional + } + for _, file := range changedFiles { + if !rule.matches(file, warningWriter) { + continue + } + target[file] = append(target[file], group) + } + } + return codeowners.NewFromFileOwners(required, optional) +} + +func (r *Rule) matches(file string, warningWriter io.Writer) bool { + for _, pattern := range r.Files { + // A directly-constructed RuleSet can carry patterns Parse would reject. + match, err := doublestar.Match(pattern, file) + if err != nil { + _, _ = fmt.Fprintf(warningWriter, "WARNING: PatternError for oracle pattern '%s': %s\n", pattern, err) + continue + } + if match { + return true + } + } + return false +} diff --git a/pkg/oracle/oracle_test.go b/pkg/oracle/oracle_test.go new file mode 100644 index 0000000..b7de33f --- /dev/null +++ b/pkg/oracle/oracle_test.go @@ -0,0 +1,205 @@ +package oracle + +import ( + "bytes" + "os" + "path/filepath" + "slices" + "strings" + "testing" + + "github.com/multimediallc/codeowners-plus/pkg/codeowners" +) + +func TestParse(t *testing.T) { + tt := []struct { + name string + input string + expectedRules int + expectedErr string + }{ + { + name: "valid rule set", + input: `{"rules": [{"files": ["a/**"], "owners": ["@team-a"], "reason": "test"}]}`, + expectedRules: 1, + }, + { + name: "empty rules", + input: `{"rules": []}`, + expectedRules: 0, + }, + { + name: "no rules key", + input: `{}`, + expectedRules: 0, + }, + { + name: "invalid JSON", + input: `{"rules": [`, + expectedErr: "invalid oracle JSON", + }, + { + name: "rule without files", + input: `{"rules": [{"files": [], "owners": ["@team-a"]}]}`, + expectedErr: "rule 0 has no files", + }, + { + name: "rule with invalid pattern", + input: `{"rules": [{"files": ["[invalid"], "owners": ["@team-a"]}]}`, + expectedErr: `rule 0 has an invalid pattern "[invalid"`, + }, + { + name: "rule with empty pattern", + input: `{"rules": [{"files": [""], "owners": ["@team-a"]}]}`, + expectedErr: `rule 0 has an invalid pattern ""`, + }, + { + name: "rule with leading-slash pattern", + input: `{"rules": [{"files": ["/src/**"], "owners": ["@team-a"]}]}`, + expectedErr: `rule 0 has a leading-slash pattern "/src/**"`, + }, + { + name: "rule without owners", + input: `{"rules": [{"files": ["a.go"], "owners": []}]}`, + expectedErr: "rule 0 has no owners", + }, + { + name: "rule with empty owner", + input: `{"rules": [{"files": ["a.go"], "owners": [""]}]}`, + expectedErr: "rule 0 has an empty owner", + }, + { + name: "rule with whitespace-only owner", + input: `{"rules": [{"files": ["a.go"], "owners": [" "]}]}`, + expectedErr: "rule 0 has an empty owner", + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + ruleSet, err := Parse([]byte(tc.input)) + if tc.expectedErr != "" { + if err == nil || !strings.Contains(err.Error(), tc.expectedErr) { + t.Fatalf("expected error containing %q, got %v", tc.expectedErr, err) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(ruleSet.Rules) != tc.expectedRules { + t.Errorf("expected %d rules, got %d", tc.expectedRules, len(ruleSet.Rules)) + } + }) + } +} + +func TestLoad(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "oracle.json") + content := `{"rules": [{"files": ["src/**"], "owners": ["@org/data-platform"], "reason": "telemetry"}]}` + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + + ruleSet, err := Load(path) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(ruleSet.Rules) != 1 || ruleSet.Rules[0].Owners[0] != "@org/data-platform" { + t.Errorf("unexpected rule set: %+v", ruleSet) + } + + if _, err := Load(filepath.Join(dir, "missing.json")); err == nil { + t.Error("expected error for missing file") + } + + badPath := filepath.Join(dir, "bad.json") + if err := os.WriteFile(badPath, []byte("not json"), 0o644); err != nil { + t.Fatal(err) + } + if _, err := Load(badPath); err == nil || !strings.Contains(err.Error(), badPath) { + t.Errorf("expected error naming the file, got %v", err) + } +} + +func requiredNames(co codeowners.CodeOwners, file string) []string { + groups, ok := co.FileRequired()[file] + if !ok { + return nil + } + return codeowners.OriginalStrings(groups.Flatten()) +} + +func TestToCodeOwners(t *testing.T) { + ruleSet := &RuleSet{Rules: []Rule{ + {Files: []string{"src/telemetry/**"}, Owners: []string{"@org/data-platform"}, Reason: "telemetry"}, + {Files: []string{"src/telemetry/events.ts"}, Owners: []string{"@org/frontend"}}, + {Files: []string{"docs/**"}, Owners: []string{"@org/docs"}, Optional: true}, + }} + changed := []string{ + "src/telemetry/events.ts", + "src/telemetry/nested/deep.py", + "docs/readme.md", + "unrelated/main.go", + } + + warnings := &bytes.Buffer{} + co := ruleSet.ToCodeOwners(changed, warnings) + + names := requiredNames(co, "src/telemetry/events.ts") + if !slices.Contains(names, "@org/data-platform") || !slices.Contains(names, "@org/frontend") { + t.Errorf("expected both oracle owners for events.ts, got %v", names) + } + if len(co.FileRequired()["src/telemetry/events.ts"]) != 2 { + t.Errorf("expected 2 AND groups for events.ts, got %d", len(co.FileRequired()["src/telemetry/events.ts"])) + } + + if names := requiredNames(co, "src/telemetry/nested/deep.py"); !slices.Contains(names, "@org/data-platform") { + t.Errorf("expected data-platform for nested file, got %v", names) + } + + if _, ok := co.FileRequired()["docs/readme.md"]; ok { + t.Error("optional rule should not create required reviewers") + } + optional := co.FileOptional()["docs/readme.md"] + if len(optional) != 1 || !slices.Contains(codeowners.OriginalStrings(optional.Flatten()), "@org/docs") { + t.Errorf("expected optional docs owner, got %v", optional) + } + + if _, ok := co.FileRequired()["unrelated/main.go"]; ok { + t.Error("unmatched file should not be tracked") + } + if len(co.UnownedFiles()) != 0 { + t.Errorf("oracle CodeOwners should report no unowned files, got %v", co.UnownedFiles()) + } + + co.ApplyApprovals([]codeowners.Slug{codeowners.NewSlug("@ORG/Data-Platform")}) + if names := requiredNames(co, "src/telemetry/nested/deep.py"); len(names) != 0 { + t.Errorf("expected no remaining required reviewers after approval, got %v", names) + } +} + +func TestToCodeOwnersNilWarningWriter(t *testing.T) { + ruleSet := &RuleSet{Rules: []Rule{ + {Files: []string{"[invalid"}, Owners: []string{"@org/data-platform"}}, + }} + co := ruleSet.ToCodeOwners([]string{"a.go"}, nil) + if len(co.FileRequired()) != 0 { + t.Errorf("bad pattern should match nothing, got %v", co.FileRequired()) + } +} + +func TestToCodeOwnersBadPattern(t *testing.T) { + ruleSet := &RuleSet{Rules: []Rule{ + {Files: []string{"[invalid"}, Owners: []string{"@org/data-platform"}}, + }} + warnings := &bytes.Buffer{} + co := ruleSet.ToCodeOwners([]string{"a.go"}, warnings) + if len(co.FileRequired()) != 0 { + t.Errorf("bad pattern should match nothing, got %v", co.FileRequired()) + } + if !strings.Contains(warnings.String(), "PatternError") { + t.Errorf("expected pattern warning, got %q", warnings.String()) + } +}