From 980c86962dc4c3aa6ec82b0bea0a9fe95507ecde Mon Sep 17 00:00:00 2001 From: zbedforrest Date: Tue, 21 Jul 2026 13:30:43 -0700 Subject: [PATCH 1/7] feat: ownership oracles (oracle-files input for externally computed reviewer requirements) --- README.md | 51 ++++++++- action.yml | 5 + internal/app/app.go | 38 +++++++ internal/app/oracle_test.go | 177 ++++++++++++++++++++++++++++++ main.go | 41 ++++--- main_test.go | 38 ++++++- pkg/codeowners/codeowners.go | 29 +++++ pkg/oracle/oracle.go | 137 +++++++++++++++++++++++ pkg/oracle/oracle_test.go | 203 +++++++++++++++++++++++++++++++++++ 9 files changed, 701 insertions(+), 18 deletions(-) create mode 100644 internal/app/oracle_test.go create mode 100644 pkg/oracle/oracle.go create mode 100644 pkg/oracle/oracle_test.go diff --git a/README.md b/README.md index 312f015..377d096 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.7%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,53 @@ 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 +* `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. +* 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 an oracle rule counts as owned, so it is not reported as an unowned file. + ## 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 e34a1dd..7d672df 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 f9f21e7..2061db3 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 @@ -163,6 +165,11 @@ func (a *App) Run() (*OutputData, error) { return &OutputData{}, fmt.Errorf("NewCodeOwners Error: %v", err) } } + // Merge in computed ownership from oracle files, if any + codeOwners, err = a.applyOracles(codeOwners, gitDiff) + if err != nil { + return &OutputData{}, err + } a.codeowners = codeOwners // Initialize user reviewer map @@ -209,6 +216,37 @@ func (a *App) Run() (*OutputData, error) { return outputData, nil } +// applyOracles AND-merges computed ownership from the configured oracle +// files into the .codeowners-derived ownership. Oracle rules can only add +// reviewer requirements, so a missing or malformed oracle file is a hard +// error: silently skipping one would drop required reviews. +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..e05817b --- /dev/null +++ b/internal/app/oracle_test.go @@ -0,0 +1,177 @@ +package app + +import ( + "bytes" + "os" + "path/filepath" + "slices" + "strings" + "testing" + + "github.com/multimediallc/codeowners-plus/pkg/codeowners" +) + +// NewFromFileOwners is covered here (rather than pkg/codeowners) because +// its consumers are the oracle and inline-ownership merge paths. +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) { + // A file .codeowners reports as unowned stops being reported once an + // oracle rule matches it (documented behavior). + 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 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..a290ffa 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,19 @@ func ignoreError[V any, E error](res V, _ E) V { return res } +// splitOracleFiles parses the comma-separated oracle-files flag, dropping +// empty entries so an unset input yields no oracle files. +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 +146,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..00509ef 100644 --- a/pkg/codeowners/codeowners.go +++ b/pkg/codeowners/codeowners.go @@ -55,6 +55,35 @@ func New(root string, files []DiffFile, fileReader FileReader, warningWriter io. return ownersMap, err } +// NewFromFileOwners creates a CodeOwners directly from file-to-reviewers +// maps, for ownership computed outside the .codeowners file tree (oracle +// rules, inline ownership blocks). Files absent from both maps are not +// tracked, so merging the result via MergeCodeOwners treats every file it +// names as owned and leaves all other files' unowned status to the other +// side of the merge. +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/oracle/oracle.go b/pkg/oracle/oracle.go new file mode 100644 index 0000000..bf4dc3b --- /dev/null +++ b/pkg/oracle/oracle.go @@ -0,0 +1,137 @@ +// Package oracle implements ownership oracles: reviewer requirements +// produced by tooling outside the .codeowners file tree (for example a +// semantic diff analyzer) and fed into codeowners-plus as data. +// +// An oracle file is JSON: +// +// { +// "rules": [ +// { +// "files": ["src/telemetry/**", "src/events.py"], +// "owners": ["@org/data-platform"], +// "optional": false, +// "reason": "telemetry event schema changed" +// } +// ] +// } +// +// Each rule's owners form a single OR group (any listed owner satisfies the +// rule), matching .codeowners semantics. Distinct rules matching the same +// file are AND-ed together. Oracle rules can only ADD reviewer requirements. +package oracle + +import ( + "encoding/json" + "fmt" + "io" + "os" + + "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 glob patterns matched against full paths of + // files changed in the PR, relative to the repository root. + Files []string `json:"files"` + // Owners is an OR group: any one of these reviewers satisfies the rule. + Owners []string `json:"owners"` + // Optional marks the owners as non-blocking (CC'd instead of required). + Optional bool `json:"optional"` + // Reason is a human-readable explanation of why the rule exists. + // It is surfaced in verbose output only. + Reason string `json:"reason"` +} + +// RuleSet is the parsed contents of an oracle file. +type RuleSet struct { + Rules []Rule `json:"rules"` +} + +// Parse decodes and validates oracle file contents. Validation is strict: +// a rule that cannot take effect (no files, no owners, or an invalid glob +// pattern) is an error, since skipping it would drop required reviews. +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 { + // An empty pattern is "valid" per doublestar but matches + // nothing, which would silently disable the rule. + if pattern == "" || !doublestar.ValidatePattern(pattern) { + return nil, fmt.Errorf("oracle rule %d has an invalid pattern %q", i, pattern) + } + } + if len(rule.Owners) == 0 { + return nil, fmt.Errorf("oracle rule %d has no owners", i) + } + for _, owner := range rule.Owners { + if owner == "" { + return nil, fmt.Errorf("oracle rule %d has an empty owner", i) + } + } + } + return &ruleSet, nil +} + +// Load reads and parses an oracle file from disk. +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 a codeowners.CodeOwners containing the rule set's +// requirements for the given changed files. Files matching no rule are +// omitted entirely, so the result is suitable for AND-merging into the +// .codeowners-derived ownership 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 { + // Parse rejects invalid patterns, but a RuleSet can also be + // constructed directly, so pattern errors are still handled. + 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..2cf810d --- /dev/null +++ b/pkg/oracle/oracle_test.go @@ -0,0 +1,203 @@ +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 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", + }, + } + + 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) + + // Two rules match events.ts: their groups AND together + 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"])) + } + + // Globstar matches nested files + 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) + } + + // Optional rules go to FileOptional, not FileRequired + 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) + } + + // Unmatched files are not tracked and not unowned + 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()) + } + + // Approvals satisfy oracle groups, case-insensitively + 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"}}, + }} + // Must not panic: the bad pattern's warning goes to io.Discard. + 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) { + // Parse rejects invalid patterns, but a RuleSet constructed directly + // can still carry one; matching warns and skips the pattern. + 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()) + } +} From afe329c72da51cc3abcb709a90d25edbc994f719 Mon Sep 17 00:00:00 2001 From: zbedforrest Date: Wed, 22 Jul 2026 13:39:08 -0700 Subject: [PATCH 2/7] fix: lowercase new error strings (staticcheck ST1005) --- README.md | 2 +- internal/app/app.go | 2 +- internal/app/oracle_test.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 377d096..a41c529 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.8%25-brightgreen) +![Coverage](https://img.shields.io/badge/Coverage-82.7%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) diff --git a/internal/app/app.go b/internal/app/app.go index cd3c06c..97721be 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -232,7 +232,7 @@ func (a *App) applyOracles(codeOwners codeowners.CodeOwners, gitDiff git.Diff) ( for _, path := range a.config.OracleFiles { ruleSet, err := oracle.Load(path) if err != nil { - return nil, fmt.Errorf("Oracle Error: %v", err) + return nil, fmt.Errorf("oracle error: %v", err) } merged.Rules = append(merged.Rules, ruleSet.Rules...) } diff --git a/internal/app/oracle_test.go b/internal/app/oracle_test.go index e05817b..9525fa8 100644 --- a/internal/app/oracle_test.go +++ b/internal/app/oracle_test.go @@ -104,7 +104,7 @@ func TestApplyOraclesMergesRequirements(t *testing.T) { 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") { + if err == nil || !strings.Contains(err.Error(), "oracle error") { t.Errorf("expected hard error for missing oracle file, got %v", err) } } From b648e7ee9a93ccab9321ecdb9f4b5c4fc51f8dcb Mon Sep 17 00:00:00 2001 From: zbedforrest Date: Mon, 10 Aug 2026 13:13:02 -0700 Subject: [PATCH 3/7] fix: address review findings on oracle fail-closed gaps - Parse rejects leading-slash patterns, which are valid per doublestar but silently never match repo-relative diff paths - Parse rejects whitespace-only owners, not just empty strings - MergeCodeOwners no longer treats optional-only reviewers as conferring ownership, so an optional oracle rule cannot suppress the unowned-file warning (matches .codeowners semantics) - README: guidance on protecting the oracle generator script from PR tampering, and clarified ownership notes --- README.md | 5 ++-- internal/app/oracle_test.go | 56 +++++++++++++++++++++++++++++++++++ pkg/codeowners/merger.go | 12 ++++++-- pkg/codeowners/merger_test.go | 18 +++++++++++ pkg/oracle/oracle.go | 8 ++++- pkg/oracle/oracle_test.go | 10 +++++++ 6 files changed, 104 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index a41c529..f5016b7 100644 --- a/README.md +++ b/README.md @@ -393,7 +393,7 @@ An oracle file is JSON, written by any earlier workflow step: } ``` -* `files`: doublestar glob patterns matched against the full repo-relative paths of files changed in the 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 @@ -418,8 +418,9 @@ Oracle requirements are AND-merged with `.codeowners` requirements, the same mec 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 an oracle rule counts as owned, so it is not reported as an unowned file. +* 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 diff --git a/internal/app/oracle_test.go b/internal/app/oracle_test.go index 9525fa8..4a31efa 100644 --- a/internal/app/oracle_test.go +++ b/internal/app/oracle_test.go @@ -163,6 +163,62 @@ func TestApplyOraclesUnownedFileBecomesOwned(t *testing.T) { } } +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}) diff --git a/pkg/codeowners/merger.go b/pkg/codeowners/merger.go index a63542c..4417e88 100644 --- a/pkg/codeowners/merger.go +++ b/pkg/codeowners/merger.go @@ -40,8 +40,16 @@ func MergeCodeOwners(base CodeOwners, head CodeOwners) CodeOwners { } } - // Merge unowned files - mergedUnowned := mergeUnownedFiles(base.UnownedFiles(), head.UnownedFiles(), allFiles) + // Merge unowned files. Only required owners confer ownership: a file + // with only optional reviewers is still unowned, matching the base + // .codeowners semantics. + 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 index bf4dc3b..3d76fec 100644 --- a/pkg/oracle/oracle.go +++ b/pkg/oracle/oracle.go @@ -25,6 +25,7 @@ import ( "fmt" "io" "os" + "strings" "github.com/bmatcuk/doublestar/v4" "github.com/multimediallc/codeowners-plus/pkg/codeowners" @@ -67,12 +68,17 @@ func Parse(data []byte) (*RuleSet, error) { if pattern == "" || !doublestar.ValidatePattern(pattern) { return nil, fmt.Errorf("oracle rule %d has an invalid pattern %q", i, pattern) } + // Doublestar patterns are already repo-root-anchored; a + // CODEOWNERS-style leading slash would silently never match. + 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 owner == "" { + if strings.TrimSpace(owner) == "" { return nil, fmt.Errorf("oracle rule %d has an empty owner", i) } } diff --git a/pkg/oracle/oracle_test.go b/pkg/oracle/oracle_test.go index 2cf810d..c779031 100644 --- a/pkg/oracle/oracle_test.go +++ b/pkg/oracle/oracle_test.go @@ -53,6 +53,11 @@ func TestParse(t *testing.T) { 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": []}]}`, @@ -63,6 +68,11 @@ func TestParse(t *testing.T) { 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 { From 63c2d2650c9fbaf9b3441bc13c2a48904b6277e3 Mon Sep 17 00:00:00 2001 From: zbedforrest Date: Mon, 10 Aug 2026 13:15:25 -0700 Subject: [PATCH 4/7] style: remove narrating comments per code conventions --- internal/app/app.go | 1 - internal/app/oracle_test.go | 2 -- pkg/codeowners/merger.go | 5 ++--- pkg/oracle/oracle_test.go | 8 -------- 4 files changed, 2 insertions(+), 14 deletions(-) diff --git a/internal/app/app.go b/internal/app/app.go index 97721be..2a287fb 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -168,7 +168,6 @@ func (a *App) Run() (*OutputData, error) { return &OutputData{}, fmt.Errorf("NewCodeOwners Error: %v", err) } } - // Merge in computed ownership from oracle files, if any codeOwners, err = a.applyOracles(codeOwners, gitDiff) if err != nil { return &OutputData{}, err diff --git a/internal/app/oracle_test.go b/internal/app/oracle_test.go index 4a31efa..fc54598 100644 --- a/internal/app/oracle_test.go +++ b/internal/app/oracle_test.go @@ -137,8 +137,6 @@ func TestApplyOraclesMultipleFiles(t *testing.T) { } func TestApplyOraclesUnownedFileBecomesOwned(t *testing.T) { - // A file .codeowners reports as unowned stops being reported once an - // oracle rule matches it (documented behavior). dir := t.TempDir() if err := os.WriteFile(filepath.Join(dir, "a.go"), []byte("package a"), 0o644); err != nil { t.Fatal(err) diff --git a/pkg/codeowners/merger.go b/pkg/codeowners/merger.go index 4417e88..dd8a0a4 100644 --- a/pkg/codeowners/merger.go +++ b/pkg/codeowners/merger.go @@ -40,9 +40,8 @@ func MergeCodeOwners(base CodeOwners, head CodeOwners) CodeOwners { } } - // Merge unowned files. Only required owners confer ownership: a file - // with only optional reviewers is still unowned, matching the base - // .codeowners semantics. + // Only required owners confer ownership: a file with only optional + // reviewers is still unowned, matching .codeowners semantics. filesWithRequiredOwners := make([]string, 0, len(mergedFileToOwner)) for file, owners := range mergedFileToOwner { if len(owners.requiredReviewers) > 0 { diff --git a/pkg/oracle/oracle_test.go b/pkg/oracle/oracle_test.go index c779031..b7de33f 100644 --- a/pkg/oracle/oracle_test.go +++ b/pkg/oracle/oracle_test.go @@ -147,7 +147,6 @@ func TestToCodeOwners(t *testing.T) { warnings := &bytes.Buffer{} co := ruleSet.ToCodeOwners(changed, warnings) - // Two rules match events.ts: their groups AND together 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) @@ -156,12 +155,10 @@ func TestToCodeOwners(t *testing.T) { t.Errorf("expected 2 AND groups for events.ts, got %d", len(co.FileRequired()["src/telemetry/events.ts"])) } - // Globstar matches nested files 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) } - // Optional rules go to FileOptional, not FileRequired if _, ok := co.FileRequired()["docs/readme.md"]; ok { t.Error("optional rule should not create required reviewers") } @@ -170,7 +167,6 @@ func TestToCodeOwners(t *testing.T) { t.Errorf("expected optional docs owner, got %v", optional) } - // Unmatched files are not tracked and not unowned if _, ok := co.FileRequired()["unrelated/main.go"]; ok { t.Error("unmatched file should not be tracked") } @@ -178,7 +174,6 @@ func TestToCodeOwners(t *testing.T) { t.Errorf("oracle CodeOwners should report no unowned files, got %v", co.UnownedFiles()) } - // Approvals satisfy oracle groups, case-insensitively 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) @@ -189,7 +184,6 @@ func TestToCodeOwnersNilWarningWriter(t *testing.T) { ruleSet := &RuleSet{Rules: []Rule{ {Files: []string{"[invalid"}, Owners: []string{"@org/data-platform"}}, }} - // Must not panic: the bad pattern's warning goes to io.Discard. co := ruleSet.ToCodeOwners([]string{"a.go"}, nil) if len(co.FileRequired()) != 0 { t.Errorf("bad pattern should match nothing, got %v", co.FileRequired()) @@ -197,8 +191,6 @@ func TestToCodeOwnersNilWarningWriter(t *testing.T) { } func TestToCodeOwnersBadPattern(t *testing.T) { - // Parse rejects invalid patterns, but a RuleSet constructed directly - // can still carry one; matching warns and skips the pattern. ruleSet := &RuleSet{Rules: []Rule{ {Files: []string{"[invalid"}, Owners: []string{"@org/data-platform"}}, }} From eee7f834044ee7056367a10614785e6b2894bbc3 Mon Sep 17 00:00:00 2001 From: zbedforrest Date: Mon, 10 Aug 2026 13:25:41 -0700 Subject: [PATCH 5/7] docs: update coverage badge to 82.8% --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index f5016b7..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.7%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) From bcada485f56e02b956938738fa54c6b689f551ef Mon Sep 17 00:00:00 2001 From: zbedforrest Date: Mon, 10 Aug 2026 13:27:05 -0700 Subject: [PATCH 6/7] style: tighten comments and docstrings --- internal/app/app.go | 7 ++--- internal/app/oracle_test.go | 2 -- main.go | 2 -- pkg/codeowners/codeowners.go | 9 ++---- pkg/codeowners/merger.go | 3 +- pkg/oracle/oracle.go | 55 ++++++++++-------------------------- 6 files changed, 22 insertions(+), 56 deletions(-) diff --git a/internal/app/app.go b/internal/app/app.go index 2a287fb..6c8f9ac 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -218,10 +218,9 @@ func (a *App) Run() (*OutputData, error) { return outputData, nil } -// applyOracles AND-merges computed ownership from the configured oracle -// files into the .codeowners-derived ownership. Oracle rules can only add -// reviewer requirements, so a missing or malformed oracle file is a hard -// error: silently skipping one would drop required reviews. +// applyOracles AND-merges ownership from the configured oracle files. +// A missing or malformed file is a hard error: silently skipping it +// would drop required reviews. func (a *App) applyOracles(codeOwners codeowners.CodeOwners, gitDiff git.Diff) (codeowners.CodeOwners, error) { if len(a.config.OracleFiles) == 0 { return codeOwners, nil diff --git a/internal/app/oracle_test.go b/internal/app/oracle_test.go index fc54598..094b9da 100644 --- a/internal/app/oracle_test.go +++ b/internal/app/oracle_test.go @@ -11,8 +11,6 @@ import ( "github.com/multimediallc/codeowners-plus/pkg/codeowners" ) -// NewFromFileOwners is covered here (rather than pkg/codeowners) because -// its consumers are the oracle and inline-ownership merge paths. func TestNewFromFileOwners(t *testing.T) { rgm := codeowners.NewReviewerGroupMemo() co := codeowners.NewFromFileOwners( diff --git a/main.go b/main.go index a290ffa..47ca223 100644 --- a/main.go +++ b/main.go @@ -76,8 +76,6 @@ func ignoreError[V any, E error](res V, _ E) V { return res } -// splitOracleFiles parses the comma-separated oracle-files flag, dropping -// empty entries so an unset input yields no oracle files. func splitOracleFiles(value string) []string { parts := strings.Split(value, ",") files := make([]string, 0, len(parts)) diff --git a/pkg/codeowners/codeowners.go b/pkg/codeowners/codeowners.go index 00509ef..b16059b 100644 --- a/pkg/codeowners/codeowners.go +++ b/pkg/codeowners/codeowners.go @@ -55,12 +55,9 @@ func New(root string, files []DiffFile, fileReader FileReader, warningWriter io. return ownersMap, err } -// NewFromFileOwners creates a CodeOwners directly from file-to-reviewers -// maps, for ownership computed outside the .codeowners file tree (oracle -// rules, inline ownership blocks). Files absent from both maps are not -// tracked, so merging the result via MergeCodeOwners treats every file it -// names as owned and leaves all other files' unowned status to the other -// side of the merge. +// NewFromFileOwners creates a CodeOwners from explicit file-to-reviewers +// maps. Files absent from both maps are untracked, leaving their unowned +// status to the other side of a MergeCodeOwners merge. func NewFromFileOwners(required map[string]ReviewerGroups, optional map[string]ReviewerGroups) CodeOwners { fileToOwner := make(map[string]fileOwners) for file, groups := range required { diff --git a/pkg/codeowners/merger.go b/pkg/codeowners/merger.go index dd8a0a4..abf34f5 100644 --- a/pkg/codeowners/merger.go +++ b/pkg/codeowners/merger.go @@ -40,8 +40,7 @@ func MergeCodeOwners(base CodeOwners, head CodeOwners) CodeOwners { } } - // Only required owners confer ownership: a file with only optional - // reviewers is still unowned, matching .codeowners semantics. + // 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 { diff --git a/pkg/oracle/oracle.go b/pkg/oracle/oracle.go index 3d76fec..3e38d71 100644 --- a/pkg/oracle/oracle.go +++ b/pkg/oracle/oracle.go @@ -1,23 +1,6 @@ -// Package oracle implements ownership oracles: reviewer requirements -// produced by tooling outside the .codeowners file tree (for example a -// semantic diff analyzer) and fed into codeowners-plus as data. -// -// An oracle file is JSON: -// -// { -// "rules": [ -// { -// "files": ["src/telemetry/**", "src/events.py"], -// "owners": ["@org/data-platform"], -// "optional": false, -// "reason": "telemetry event schema changed" -// } -// ] -// } -// -// Each rule's owners form a single OR group (any listed owner satisfies the -// rule), matching .codeowners semantics. Distinct rules matching the same -// file are AND-ed together. Oracle rules can only ADD reviewer requirements. +// Package oracle loads reviewer requirements computed by external tooling +// (JSON format documented in the README under "Ownership Oracles"). Oracle +// rules can only add reviewer requirements, never weaken .codeowners ones. package oracle import ( @@ -33,15 +16,13 @@ import ( // Rule is a single oracle reviewer requirement. type Rule struct { - // Files are doublestar glob patterns matched against full paths of - // files changed in the PR, relative to the repository root. + // Files are doublestar globs matched against repo-relative changed paths. Files []string `json:"files"` - // Owners is an OR group: any one of these reviewers satisfies the rule. + // Owners is an OR group: any one listed reviewer satisfies the rule. Owners []string `json:"owners"` - // Optional marks the owners as non-blocking (CC'd instead of required). + // Optional owners are CC'd instead of required. Optional bool `json:"optional"` - // Reason is a human-readable explanation of why the rule exists. - // It is surfaced in verbose output only. + // Reason is surfaced in verbose output. Reason string `json:"reason"` } @@ -50,9 +31,8 @@ type RuleSet struct { Rules []Rule `json:"rules"` } -// Parse decodes and validates oracle file contents. Validation is strict: -// a rule that cannot take effect (no files, no owners, or an invalid glob -// pattern) is an error, since skipping it would drop required reviews. +// Parse decodes and validates oracle JSON. A rule that cannot take effect +// is an error, since silently skipping it would drop required reviews. func Parse(data []byte) (*RuleSet, error) { var ruleSet RuleSet if err := json.Unmarshal(data, &ruleSet); err != nil { @@ -63,13 +43,11 @@ func Parse(data []byte) (*RuleSet, error) { return nil, fmt.Errorf("oracle rule %d has no files", i) } for _, pattern := range rule.Files { - // An empty pattern is "valid" per doublestar but matches - // nothing, which would silently disable the rule. + // 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 patterns are already repo-root-anchored; a - // CODEOWNERS-style leading slash would silently never match. + // 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) } @@ -86,7 +64,7 @@ func Parse(data []byte) (*RuleSet, error) { return &ruleSet, nil } -// Load reads and parses an oracle file from disk. +// Load reads and parses an oracle file. func Load(path string) (*RuleSet, error) { data, err := os.ReadFile(path) if err != nil { @@ -99,10 +77,8 @@ func Load(path string) (*RuleSet, error) { return ruleSet, nil } -// ToCodeOwners builds a codeowners.CodeOwners containing the rule set's -// requirements for the given changed files. Files matching no rule are -// omitted entirely, so the result is suitable for AND-merging into the -// .codeowners-derived ownership via codeowners.MergeCodeOwners. +// ToCodeOwners builds the rule set's requirements for the changed files, +// suitable for AND-merging via codeowners.MergeCodeOwners. func (rs *RuleSet) ToCodeOwners(changedFiles []string, warningWriter io.Writer) codeowners.CodeOwners { if warningWriter == nil { warningWriter = io.Discard @@ -128,8 +104,7 @@ func (rs *RuleSet) ToCodeOwners(changedFiles []string, warningWriter io.Writer) func (r *Rule) matches(file string, warningWriter io.Writer) bool { for _, pattern := range r.Files { - // Parse rejects invalid patterns, but a RuleSet can also be - // constructed directly, so pattern errors are still handled. + // 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) From 59df5ffd890110844cf9b40a169b2750a398f84c Mon Sep 17 00:00:00 2001 From: zbedforrest Date: Mon, 10 Aug 2026 13:29:44 -0700 Subject: [PATCH 7/7] style: single-line docstrings --- internal/app/app.go | 4 +--- pkg/codeowners/codeowners.go | 4 +--- pkg/oracle/oracle.go | 10 +++------- 3 files changed, 5 insertions(+), 13 deletions(-) diff --git a/internal/app/app.go b/internal/app/app.go index 6c8f9ac..abeb110 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -218,9 +218,7 @@ 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: silently skipping it -// would drop required reviews. +// 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 diff --git a/pkg/codeowners/codeowners.go b/pkg/codeowners/codeowners.go index b16059b..6fab76f 100644 --- a/pkg/codeowners/codeowners.go +++ b/pkg/codeowners/codeowners.go @@ -55,9 +55,7 @@ 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, leaving their unowned -// status to the other side of a MergeCodeOwners merge. +// 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 { diff --git a/pkg/oracle/oracle.go b/pkg/oracle/oracle.go index 3e38d71..76fc585 100644 --- a/pkg/oracle/oracle.go +++ b/pkg/oracle/oracle.go @@ -1,6 +1,4 @@ -// Package oracle loads reviewer requirements computed by external tooling -// (JSON format documented in the README under "Ownership Oracles"). Oracle -// rules can only add reviewer requirements, never weaken .codeowners ones. +// Package oracle loads externally computed reviewer requirements (see "Ownership Oracles" in the README). package oracle import ( @@ -31,8 +29,7 @@ type RuleSet struct { Rules []Rule `json:"rules"` } -// Parse decodes and validates oracle JSON. A rule that cannot take effect -// is an error, since silently skipping it would drop required reviews. +// 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 { @@ -77,8 +74,7 @@ func Load(path string) (*RuleSet, error) { return ruleSet, nil } -// ToCodeOwners builds the rule set's requirements for the changed files, -// suitable for AND-merging via codeowners.MergeCodeOwners. +// 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