From ec7642b048f21481de456e904ab7cde5b9837448 Mon Sep 17 00:00:00 2001 From: BakerNet Date: Thu, 30 Oct 2025 16:13:01 -0700 Subject: [PATCH 1/4] Update docs to use base_ref instead of head for actions/checkout --- .github/workflows/codeowners.yml | 1 + README.md | 1 + 2 files changed, 2 insertions(+) diff --git a/.github/workflows/codeowners.yml b/.github/workflows/codeowners.yml index dee7659..dfa4ccd 100644 --- a/.github/workflows/codeowners.yml +++ b/.github/workflows/codeowners.yml @@ -23,6 +23,7 @@ jobs: - name: 'Checkout Code Repository' uses: actions/checkout@v5 with: + ref: ${{ github.base_ref }} fetch-depth: 0 - name: 'Update action.yml to build locally' diff --git a/README.md b/README.md index 3e56467..d9c73b6 100644 --- a/README.md +++ b/README.md @@ -89,6 +89,7 @@ jobs: - name: 'Checkout Code Repository' uses: actions/checkout@v4 with: + ref: ${{ github.base_ref }} # use the base_ref to prevent bypass by PR author fetch-depth: 0 - name: 'Codeowners Plus' From 2b3e4b6de2c95c9b16bb629b7f83234fc9192516 Mon Sep 17 00:00:00 2001 From: BakerNet Date: Thu, 30 Oct 2025 17:36:40 -0700 Subject: [PATCH 2/4] Read from the base ref using git instead of filesystem --- .github/workflows/codeowners.yml | 1 - README.md | 1 - internal/app/app.go | 19 +-- internal/config/config.go | 37 +++--- internal/config/config_git_ref_test.go | 155 +++++++++++++++++++++++++ internal/config/config_test.go | 4 +- internal/git/file_reader.go | 45 +++++++ internal/git/file_reader_test.go | 142 ++++++++++++++++++++++ pkg/codeowners/codeowners.go | 12 +- pkg/codeowners/codeowners_test.go | 12 +- pkg/codeowners/reader.go | 54 +++++++-- pkg/codeowners/reader_test.go | 18 +-- tools/cli/main.go | 8 +- 13 files changed, 447 insertions(+), 61 deletions(-) create mode 100644 internal/config/config_git_ref_test.go create mode 100644 internal/git/file_reader.go create mode 100644 internal/git/file_reader_test.go diff --git a/.github/workflows/codeowners.yml b/.github/workflows/codeowners.yml index dfa4ccd..dee7659 100644 --- a/.github/workflows/codeowners.yml +++ b/.github/workflows/codeowners.yml @@ -23,7 +23,6 @@ jobs: - name: 'Checkout Code Repository' uses: actions/checkout@v5 with: - ref: ${{ github.base_ref }} fetch-depth: 0 - name: 'Update action.yml to build locally' diff --git a/README.md b/README.md index d9c73b6..3e56467 100644 --- a/README.md +++ b/README.md @@ -89,7 +89,6 @@ jobs: - name: 'Checkout Code Repository' uses: actions/checkout@v4 with: - ref: ${{ github.base_ref }} # use the base_ref to prevent bypass by PR author fetch-depth: 0 - name: 'Codeowners Plus' diff --git a/internal/app/app.go b/internal/app/app.go index a4b4b4a..96a59d0 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -106,8 +106,13 @@ func (a *App) Run() (*OutputData, error) { } a.printDebug("PR: %d\n", a.client.PR().GetNumber()) - // Read config - conf, err := owners.ReadConfig(a.config.RepoDir) + // Create file reader for base ref to prevent PR authors from modifying config or .codeowners + // This ensures the security policy comes from the protected branch, not the PR branch + fileReader := git.NewGitRefFileReader(a.client.PR().Base.GetSHA(), a.config.RepoDir) + a.printDebug("Using base ref %s for codeowners.toml and .codeowners files\n", a.client.PR().Base.GetSHA()) + + // Read config from base ref + conf, err := owners.ReadConfig(a.config.RepoDir, fileReader) if err != nil { a.printWarn("Error reading codeowners.toml - using default config\n") } @@ -129,8 +134,8 @@ func (a *App) Run() (*OutputData, error) { } a.gitDiff = gitDiff - // Initialize codeowners - codeOwners, err := codeowners.New(a.config.RepoDir, gitDiff.AllChanges(), a.config.WarningBuffer) + // Initialize codeowners using base ref file reader (same reader as config) + codeOwners, err := codeowners.New(a.config.RepoDir, gitDiff.AllChanges(), fileReader, a.config.WarningBuffer) if err != nil { return &OutputData{}, fmt.Errorf("NewCodeOwners Error: %v", err) } @@ -402,10 +407,10 @@ func (a *App) processTokenOwnerApproval() (*gh.CurrentApproval, error) { func (a *App) processApprovals(ghApprovals []*gh.CurrentApproval) (int, error) { fileReviewers := f.MapMap(a.codeowners.FileRequired(), func(reviewers codeowners.ReviewerGroups) []string { return reviewers.Flatten() }) - + var approvers []string var approvalsToDismiss []*gh.CurrentApproval - + if a.Conf.DisableSmartDismissal { // Smart dismissal is disabled - treat all approvals as valid a.printDebug("Smart dismissal disabled - keeping all approvals\n") @@ -417,7 +422,7 @@ func (a *App) processApprovals(ghApprovals []*gh.CurrentApproval) (int, error) { // Normal smart dismissal logic approvers, approvalsToDismiss = a.client.CheckApprovals(fileReviewers, ghApprovals, a.gitDiff) } - + a.codeowners.ApplyApprovals(approvers) if len(approvalsToDismiss) > 0 { diff --git a/internal/config/config.go b/internal/config/config.go index 2ae41bd..0514246 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1,23 +1,22 @@ package owners import ( - "errors" - "os" "strings" + "github.com/multimediallc/codeowners-plus/pkg/codeowners" "github.com/pelletier/go-toml/v2" ) type Config struct { - MaxReviews *int `toml:"max_reviews"` - MinReviews *int `toml:"min_reviews"` - UnskippableReviewers []string `toml:"unskippable_reviewers"` - Ignore []string `toml:"ignore"` - Enforcement *Enforcement `toml:"enforcement"` - HighPriorityLabels []string `toml:"high_priority_labels"` - AdminBypass *AdminBypass `toml:"admin_bypass"` - DetailedReviewers bool `toml:"detailed_reviewers"` - DisableSmartDismissal bool `toml:"disable_smart_dismissal"` + MaxReviews *int `toml:"max_reviews"` + MinReviews *int `toml:"min_reviews"` + UnskippableReviewers []string `toml:"unskippable_reviewers"` + Ignore []string `toml:"ignore"` + Enforcement *Enforcement `toml:"enforcement"` + HighPriorityLabels []string `toml:"high_priority_labels"` + AdminBypass *AdminBypass `toml:"admin_bypass"` + DetailedReviewers bool `toml:"detailed_reviewers"` + DisableSmartDismissal bool `toml:"disable_smart_dismissal"` } type Enforcement struct { @@ -26,11 +25,11 @@ type Enforcement struct { } type AdminBypass struct { - Enabled bool `toml:"enabled"` - AllowedUsers []string `toml:"allowed_users"` + Enabled bool `toml:"enabled"` + AllowedUsers []string `toml:"allowed_users"` } -func ReadConfig(path string) (*Config, error) { +func ReadConfig(path string, fileReader codeowners.FileReader) (*Config, error) { if !strings.HasSuffix(path, "/") { path += "/" } @@ -46,11 +45,17 @@ func ReadConfig(path string) (*Config, error) { DetailedReviewers: false, } + // Use filesystem reader if none provided + if fileReader == nil { + fileReader = &codeowners.FilesystemReader{} + } + fileName := path + "codeowners.toml" - if _, err := os.Stat(fileName); errors.Is(err, os.ErrNotExist) { + + if !fileReader.PathExists(fileName) { return defaultConfig, nil } - file, err := os.ReadFile(fileName) + file, err := fileReader.ReadFile(fileName) if err != nil { return defaultConfig, err } diff --git a/internal/config/config_git_ref_test.go b/internal/config/config_git_ref_test.go new file mode 100644 index 0000000..9b81f41 --- /dev/null +++ b/internal/config/config_git_ref_test.go @@ -0,0 +1,155 @@ +package owners + +import ( + "fmt" + "strings" + "testing" +) + +type mockConfigFileReader struct { + files map[string]string +} + +func (m *mockConfigFileReader) ReadFile(path string) ([]byte, error) { + content, ok := m.files[path] + if !ok { + return nil, fmt.Errorf("file not found: %s", path) + } + return []byte(content), nil +} + +func (m *mockConfigFileReader) PathExists(path string) bool { + _, ok := m.files[path] + return ok +} + +func TestReadConfigFromGitRef(t *testing.T) { + // Simulate reading config from a git ref + mockReader := &mockConfigFileReader{ + files: map[string]string{ + "test/repo/codeowners.toml": ` +max_reviews = 3 +min_reviews = 2 +unskippable_reviewers = ["@admin"] + +[enforcement] +approval = true +fail_check = false +`, + }, + } + + config, err := ReadConfig("test/repo", mockReader) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if config.MaxReviews == nil || *config.MaxReviews != 3 { + t.Errorf("expected max_reviews = 3, got %v", config.MaxReviews) + } + + if config.MinReviews == nil || *config.MinReviews != 2 { + t.Errorf("expected min_reviews = 2, got %v", config.MinReviews) + } + + if len(config.UnskippableReviewers) != 1 || config.UnskippableReviewers[0] != "@admin" { + t.Errorf("expected unskippable_reviewers = [@admin], got %v", config.UnskippableReviewers) + } + + if !config.Enforcement.Approval { + t.Error("expected enforcement.approval = true") + } + + if config.Enforcement.FailCheck { + t.Error("expected enforcement.fail_check = false") + } +} + +func TestReadConfigFromGitRefNotFound(t *testing.T) { + // Simulate config file not existing in git ref + mockReader := &mockConfigFileReader{ + files: map[string]string{}, + } + + config, err := ReadConfig("test/repo", mockReader) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Should return default config + if config.MaxReviews != nil { + t.Error("expected nil max_reviews for default config") + } + + if config.MinReviews != nil { + t.Error("expected nil min_reviews for default config") + } + + if config.Enforcement.Approval { + t.Error("expected enforcement.approval = false for default config") + } + + if !config.Enforcement.FailCheck { + t.Error("expected enforcement.fail_check = true for default config") + } +} + +func TestReadConfigFromGitRefInvalidToml(t *testing.T) { + // Simulate invalid TOML content + mockReader := &mockConfigFileReader{ + files: map[string]string{ + "test/repo/codeowners.toml": "invalid toml [[[", + }, + } + + _, err := ReadConfig("test/repo", mockReader) + if err == nil { + t.Error("expected error for invalid TOML") + } + + if !strings.Contains(err.Error(), "toml") && !strings.Contains(err.Error(), "parse") { + t.Errorf("expected TOML parse error, got: %v", err) + } +} + +func TestReadConfigUsesFilesystemWhenNilReader(t *testing.T) { + // When nil reader is passed, it should use filesystem reader + // This test just ensures the fallback logic works + config, err := ReadConfig("../../test_project", nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // The test_project directory might not have a codeowners.toml + // In that case, we should get default config + if config.Enforcement == nil { + t.Error("expected enforcement config to be initialized") + } +} + +// TestConfigSecurityIsolation verifies that even if filesystem has different config, +// the git ref reader returns the correct config from the ref +func TestConfigSecurityIsolation(t *testing.T) { + // This simulates the security scenario where: + // - Base ref has max_reviews = 5 + // - PR branch (filesystem) has max_reviews = 1 (trying to bypass) + // - We should only see the base ref config (max_reviews = 5) + + mockReader := &mockConfigFileReader{ + files: map[string]string{ + "test/repo/codeowners.toml": "max_reviews = 5", + }, + } + + config, err := ReadConfig("test/repo", mockReader) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if config.MaxReviews == nil || *config.MaxReviews != 5 { + t.Errorf("expected max_reviews = 5 from base ref, got %v", config.MaxReviews) + } + + // Even if filesystem has different value, we're reading from git ref + // so we should only see the base ref value +} diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 4c8038c..68a40e2 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -100,7 +100,7 @@ max_reviews = invalid // Test with and without trailing slash paths := []string{configPath, configPath + "/"} for _, path := range paths { - got, err := ReadConfig(path) + got, err := ReadConfig(path, nil) if tc.expectedErr { if err == nil { t.Error("expected error but got none") @@ -176,7 +176,7 @@ func TestReadConfigFileError(t *testing.T) { } // Try to read config from directory with no permissions - _, err = ReadConfig(configPath) + _, err = ReadConfig(configPath, nil) if err == nil { t.Error("expected error when reading from directory with no permissions") } diff --git a/internal/git/file_reader.go b/internal/git/file_reader.go new file mode 100644 index 0000000..264e154 --- /dev/null +++ b/internal/git/file_reader.go @@ -0,0 +1,45 @@ +package git + +import ( + "fmt" + "strings" +) + +// GitRefFileReader reads files from a specific git ref +type GitRefFileReader struct { + ref string + dir string + executor gitCommandExecutor +} + +// NewGitRefFileReader creates a new GitRefFileReader for reading files from a git ref +func NewGitRefFileReader(ref string, dir string) *GitRefFileReader { + return &GitRefFileReader{ + ref: ref, + dir: dir, + executor: newRealGitExecutor(dir), + } +} + +// ReadFile reads a file from the git ref +func (r *GitRefFileReader) ReadFile(path string) ([]byte, error) { + // Normalize path - remove leading slash if present + path = strings.TrimPrefix(path, "/") + + // Use git show to read the file from the ref + output, err := r.executor.execute("git", "show", fmt.Sprintf("%s:%s", r.ref, path)) + if err != nil { + return nil, fmt.Errorf("failed to read file %s from ref %s: %w", path, r.ref, err) + } + return output, nil +} + +// PathExists checks if a file exists in the git ref +func (r *GitRefFileReader) PathExists(path string) bool { + // Normalize path - remove leading slash if present + path = strings.TrimPrefix(path, "/") + + // Use git cat-file to check if the file exists + _, err := r.executor.execute("git", "cat-file", "-e", fmt.Sprintf("%s:%s", r.ref, path)) + return err == nil +} diff --git a/internal/git/file_reader_test.go b/internal/git/file_reader_test.go new file mode 100644 index 0000000..5bf5709 --- /dev/null +++ b/internal/git/file_reader_test.go @@ -0,0 +1,142 @@ +package git + +import ( + "fmt" + "strings" + "testing" +) + +type mockFileReaderExecutor struct { + outputs map[string][]byte + errors map[string]error +} + +func (m *mockFileReaderExecutor) execute(command string, args ...string) ([]byte, error) { + key := fmt.Sprintf("%s %s", command, strings.Join(args, " ")) + if err, ok := m.errors[key]; ok { + return nil, err + } + if output, ok := m.outputs[key]; ok { + return output, nil + } + return nil, fmt.Errorf("unexpected command: %s", key) +} + +func TestGitRefFileReader_ReadFile(t *testing.T) { + mockExec := &mockFileReaderExecutor{ + outputs: map[string][]byte{ + "git show baseref123:.codeowners": []byte("* @owner1\n"), + "git show baseref123:subdir/.codeowners": []byte("*.js @owner2\n"), + }, + errors: map[string]error{ + "git show baseref123:nonexistent": fmt.Errorf("file not found"), + }, + } + + reader := &GitRefFileReader{ + ref: "baseref123", + dir: "/repo", + executor: mockExec, + } + + tt := []struct { + name string + path string + expected string + expectError bool + }{ + { + name: "read root codeowners", + path: ".codeowners", + expected: "* @owner1\n", + expectError: false, + }, + { + name: "read subdirectory codeowners", + path: "subdir/.codeowners", + expected: "*.js @owner2\n", + expectError: false, + }, + { + name: "read nonexistent file", + path: "nonexistent", + expectError: true, + }, + { + name: "read with leading slash", + path: "/.codeowners", + expected: "* @owner1\n", + expectError: false, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + content, err := reader.ReadFile(tc.path) + + if tc.expectError { + if err == nil { + t.Errorf("expected error but got none") + } + return + } + + if err != nil { + t.Errorf("unexpected error: %v", err) + return + } + + if string(content) != tc.expected { + t.Errorf("expected %q, got %q", tc.expected, string(content)) + } + }) + } +} + +func TestGitRefFileReader_PathExists(t *testing.T) { + mockExec := &mockFileReaderExecutor{ + outputs: map[string][]byte{ + "git cat-file -e baseref123:.codeowners": []byte(""), + }, + errors: map[string]error{ + "git cat-file -e baseref123:nonexistent": fmt.Errorf("file not found"), + }, + } + + reader := &GitRefFileReader{ + ref: "baseref123", + dir: "/repo", + executor: mockExec, + } + + tt := []struct { + name string + path string + expected bool + }{ + { + name: "existing file", + path: ".codeowners", + expected: true, + }, + { + name: "nonexistent file", + path: "nonexistent", + expected: false, + }, + { + name: "existing file with leading slash", + path: "/.codeowners", + expected: true, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + exists := reader.PathExists(tc.path) + if exists != tc.expected { + t.Errorf("expected %v, got %v", tc.expected, exists) + } + }) + } +} diff --git a/pkg/codeowners/codeowners.go b/pkg/codeowners/codeowners.go index 070c943..82f6c6c 100644 --- a/pkg/codeowners/codeowners.go +++ b/pkg/codeowners/codeowners.go @@ -33,9 +33,10 @@ type CodeOwners interface { } // New creates a new CodeOwners object from a root path and a list of diff files -func New(root string, files []DiffFile, warningWriter io.Writer) (CodeOwners, error) { +// If fileReader is nil, it will use the filesystem +func New(root string, files []DiffFile, fileReader FileReader, warningWriter io.Writer) (CodeOwners, error) { reviewerGroupManager := NewReviewerGroupMemo() - tree := initOwnerTreeNode(root, root, reviewerGroupManager, nil, warningWriter) + tree := initOwnerTreeNode(root, root, reviewerGroupManager, nil, fileReader, warningWriter) tree.warningWriter = warningWriter // TODO - support inline ownership rules (issue #3) fileNames := f.Map(files, func(file DiffFile) string { return file.FileName }) @@ -125,6 +126,7 @@ type ownerTreeNode struct { optionalReviewerTests FileTestCases fallback *ReviewerGroup warningWriter io.Writer + fileReader FileReader } func initOwnerTreeNode( @@ -132,9 +134,10 @@ func initOwnerTreeNode( path string, reviewerGroupManager ReviewerGroupManager, parent *ownerTreeNode, + fileReader FileReader, warningWriter io.Writer, ) *ownerTreeNode { - rules := Read(path, reviewerGroupManager, warningWriter) + rules := Read(path, reviewerGroupManager, fileReader, warningWriter) fallback := rules.Fallback ownerTests := rules.OwnerTests additionalReviewerTests := rules.AdditionalReviewerTests @@ -154,6 +157,7 @@ func initOwnerTreeNode( optionalReviewerTests: optionalReviewerTests, fallback: fallback, warningWriter: io.Discard, + fileReader: fileReader, } } @@ -172,7 +176,7 @@ func (tree *ownerTreeNode) BuildFromFiles( currPath = currPath + "/" + part partNode, ok := currNode.children[part] if !ok { - partNode = initOwnerTreeNode(part, currPath, reviewerGroupManager, currNode, tree.warningWriter) + partNode = initOwnerTreeNode(part, currPath, reviewerGroupManager, currNode, tree.fileReader, tree.warningWriter) currNode.children[part] = partNode } currNode = partNode diff --git a/pkg/codeowners/codeowners_test.go b/pkg/codeowners/codeowners_test.go index 3b38998..05921cf 100644 --- a/pkg/codeowners/codeowners_test.go +++ b/pkg/codeowners/codeowners_test.go @@ -10,7 +10,7 @@ import ( func TestInitOwnerTree(t *testing.T) { rgMan := NewReviewerGroupMemo() - tree := initOwnerTreeNode("../../test_project", "../../test_project", rgMan, nil, io.Discard) + tree := initOwnerTreeNode("../../test_project", "../../test_project", rgMan, nil, nil, io.Discard) if tree.name != "../../test_project" { t.Errorf("Expected name to be ../test_project, got %s", tree.name) @@ -64,7 +64,7 @@ func TestGetOwners(t *testing.T) { "frontend/inner/a.test.js", } rgMan := NewReviewerGroupMemo() - tree := initOwnerTreeNode("../../test_project", "../../test_project", rgMan, nil, io.Discard) + tree := initOwnerTreeNode("../../test_project", "../../test_project", rgMan, nil, nil, io.Discard) testMap := tree.BuildFromFiles(files, rgMan) owners, err := testMap.getOwners(files) if err != nil { @@ -117,7 +117,7 @@ func TestGetOwners(t *testing.T) { func TestGetOwnersFail(t *testing.T) { files := []string{"a.py"} rgMan := NewReviewerGroupMemo() - tree := initOwnerTreeNode("../../test_project", "../../test_project", rgMan, nil, io.Discard) + tree := initOwnerTreeNode("../../test_project", "../../test_project", rgMan, nil, nil, io.Discard) testMap := tree.BuildFromFiles(files, rgMan) files = append(files, "non_existent_file") _, err := testMap.getOwners(files) @@ -129,7 +129,7 @@ func TestGetOwnersFail(t *testing.T) { func TestGetOwnersNoFallback(t *testing.T) { files := []string{"a.py"} rgMan := NewReviewerGroupMemo() - tree := initOwnerTreeNode("../../test_project", "../../test_project", rgMan, nil, io.Discard) + tree := initOwnerTreeNode("../../test_project", "../../test_project", rgMan, nil, nil, io.Discard) testMap := tree.BuildFromFiles(files, rgMan) testMap["a.py"].fallback = nil owners, err := testMap.getOwners(files) @@ -147,7 +147,7 @@ func TestNewCodeOwners(t *testing.T) { {FileName: "frontend/b.ts"}, {FileName: "frontend/inner/a.test.js"}, } - _, err := New("../../test_project", files, io.Discard) + _, err := New("../../test_project", files, nil, io.Discard) if err != nil { t.Errorf("NewCodeOwners error: %v", err) } @@ -164,7 +164,7 @@ func setupOwnersMap() (*ownersMap, map[string]bool, error) { "frontend/inner/a.js", } rgMan := NewReviewerGroupMemo() - tree := initOwnerTreeNode("../../test_project", "../../test_project", rgMan, nil, io.Discard) + tree := initOwnerTreeNode("../../test_project", "../../test_project", rgMan, nil, nil, io.Discard) testMap := tree.BuildFromFiles(files, rgMan) owners, error := testMap.getOwners(files) expectedOwners := map[string]bool{ diff --git a/pkg/codeowners/reader.go b/pkg/codeowners/reader.go index 7839da2..b533bce 100644 --- a/pkg/codeowners/reader.go +++ b/pkg/codeowners/reader.go @@ -2,6 +2,8 @@ package codeowners import ( "bufio" + "bytes" + "errors" "fmt" "io" "os" @@ -10,6 +12,24 @@ import ( "strings" ) +// FileReader defines an interface for reading files from different sources +type FileReader interface { + ReadFile(path string) ([]byte, error) + PathExists(path string) bool +} + +// FilesystemReader implements FileReader for the local filesystem +type FilesystemReader struct{} + +func (f *FilesystemReader) ReadFile(path string) ([]byte, error) { + return os.ReadFile(path) +} + +func (f *FilesystemReader) PathExists(path string) bool { + _, err := os.Stat(path) + return !errors.Is(err, os.ErrNotExist) +} + type Rules struct { Fallback *ReviewerGroup OwnerTests FileTestCases @@ -18,7 +38,8 @@ type Rules struct { } // Read the .codeowners file and return the fallback owner, ownership tests, and additional ownership tests -func Read(path string, reviewerGroupManager ReviewerGroupManager, warningWriter io.Writer) Rules { +// If fileReader is nil, it will use the filesystem +func Read(path string, reviewerGroupManager ReviewerGroupManager, fileReader FileReader, warningWriter io.Writer) Rules { rules := Rules{ Fallback: nil, OwnerTests: FileTestCases{}, @@ -26,26 +47,37 @@ func Read(path string, reviewerGroupManager ReviewerGroupManager, warningWriter OptionalReviewerTests: FileTestCases{}, } - ls, error := os.Lstat(path) - if error != nil || !ls.IsDir() { - return rules + // Use filesystem reader if none provided + if fileReader == nil { + fileReader = &FilesystemReader{} + } + + // For filesystem reader, check if path is a directory + if _, ok := fileReader.(*FilesystemReader); ok { + ls, error := os.Lstat(path) + if error != nil || !ls.IsDir() { + return rules + } } if !strings.HasSuffix(path, "/") { path += "/" } - file, err := os.Open(path + ".codeowners") + codeownersPath := path + ".codeowners" + + // Check if the .codeowners file exists + if !fileReader.PathExists(codeownersPath) { + return rules + } + + // Read the file content + content, err := fileReader.ReadFile(codeownersPath) if err != nil { return rules } - defer func() { - if err := file.Close(); err != nil { - _, _ = fmt.Fprintf(warningWriter, "WARNING: Error closing file: %v\n", err) - } - }() - scanner := bufio.NewScanner(file) + scanner := bufio.NewScanner(bytes.NewReader(content)) for scanner.Scan() { line := scanner.Text() line = strings.TrimSpace(line) diff --git a/pkg/codeowners/reader_test.go b/pkg/codeowners/reader_test.go index d33d4ae..04fe47b 100644 --- a/pkg/codeowners/reader_test.go +++ b/pkg/codeowners/reader_test.go @@ -34,18 +34,18 @@ func TestRead(t *testing.T) { optional: 0, }, { - name: "non-directory file", - path: "../../test_project/a.py", - fallback: false, - owners: 0, + name: "non-directory file", + path: "../../test_project/a.py", + fallback: false, + owners: 0, additional: 0, optional: 0, }, { - name: "empty directory", - path: "../../test_project/empty", - fallback: false, - owners: 0, + name: "empty directory", + path: "../../test_project/empty", + fallback: false, + owners: 0, additional: 0, optional: 0, }, @@ -54,7 +54,7 @@ func TestRead(t *testing.T) { rgMan := NewReviewerGroupMemo() for _, tc := range tt { t.Run(tc.name, func(t *testing.T) { - rules := Read(tc.path, rgMan, io.Discard) + rules := Read(tc.path, rgMan, nil, io.Discard) if !tc.fallback && rules.Fallback != nil { t.Errorf("Expected fallback to be nil, got %+v", rules.Fallback) diff --git a/tools/cli/main.go b/tools/cli/main.go index 6e90b6e..7ede1cf 100644 --- a/tools/cli/main.go +++ b/tools/cli/main.go @@ -287,7 +287,7 @@ func unownedFilesWithFormat(repo string, targets []string, depth int, dirsOnly b filesForTarget = append(filesForTarget, repoFile) } - ownersMap, err := codeowners.New(repo, filesForTarget, io.Discard) + ownersMap, err := codeowners.New(repo, filesForTarget, &codeowners.FilesystemReader{}, io.Discard) if err != nil { return fmt.Errorf("error reading codeowners config: %s", err) } @@ -433,7 +433,7 @@ func fileOwner(repo string, targets []string, format OutputFormat) error { diffFiles[i] = codeowners.DiffFile{FileName: target} } - ownersMap, err := codeowners.New(repo, diffFiles, io.Discard) + ownersMap, err := codeowners.New(repo, diffFiles, &codeowners.FilesystemReader{}, io.Discard) if err != nil { return fmt.Errorf("error reading codeowners config: %s", err) } @@ -464,7 +464,7 @@ func generateOwnershipMap(repo string, mapBy string) error { return err } - ownersMap, err := codeowners.New(repo, files, io.Discard) + ownersMap, err := codeowners.New(repo, files, &codeowners.FilesystemReader{}, io.Discard) if err != nil { return fmt.Errorf("error reading codeowners config: %s", err) } @@ -562,7 +562,7 @@ func validateCodeowners(repo string, target string) error { rgm := codeowners.NewReviewerGroupMemo() - codeowners := codeowners.Read(target, rgm, io.Discard) + codeowners := codeowners.Read(target, rgm, &codeowners.FilesystemReader{}, io.Discard) if codeowners.Fallback != nil { for _, name := range codeowners.Fallback.Names { if !strings.HasPrefix(name, "@") { From 2062c50b07806fba7256783e5c587a99bbcb76ef Mon Sep 17 00:00:00 2001 From: BakerNet Date: Thu, 30 Oct 2025 18:18:22 -0700 Subject: [PATCH 3/4] Fix Git path normalization --- internal/git/file_reader.go | 22 ++++- .../file_reader_path_normalization_test.go | 80 +++++++++++++++++++ internal/git/file_reader_test.go | 17 ++++ 3 files changed, 115 insertions(+), 4 deletions(-) create mode 100644 internal/git/file_reader_path_normalization_test.go diff --git a/internal/git/file_reader.go b/internal/git/file_reader.go index 264e154..0525463 100644 --- a/internal/git/file_reader.go +++ b/internal/git/file_reader.go @@ -23,8 +23,8 @@ func NewGitRefFileReader(ref string, dir string) *GitRefFileReader { // ReadFile reads a file from the git ref func (r *GitRefFileReader) ReadFile(path string) ([]byte, error) { - // Normalize path - remove leading slash if present - path = strings.TrimPrefix(path, "/") + // Normalize path - make it relative to the repository root + path = r.normalizePathForGit(path) // Use git show to read the file from the ref output, err := r.executor.execute("git", "show", fmt.Sprintf("%s:%s", r.ref, path)) @@ -36,10 +36,24 @@ func (r *GitRefFileReader) ReadFile(path string) ([]byte, error) { // PathExists checks if a file exists in the git ref func (r *GitRefFileReader) PathExists(path string) bool { - // Normalize path - remove leading slash if present - path = strings.TrimPrefix(path, "/") + // Normalize path - make it relative to the repository root + path = r.normalizePathForGit(path) // Use git cat-file to check if the file exists _, err := r.executor.execute("git", "cat-file", "-e", fmt.Sprintf("%s:%s", r.ref, path)) return err == nil } + +// normalizePathForGit converts an absolute filesystem path to a path relative to the repository root +func (r *GitRefFileReader) normalizePathForGit(path string) string { + // We want to remove the path to the dir Root + dir_prefix := r.dir + if !strings.HasSuffix(dir_prefix, "/") { + dir_prefix = dir_prefix + "/" + } + path = strings.TrimPrefix(path, dir_prefix) + if strings.HasPrefix(path, "/") { + return path[1:] + } + return path +} diff --git a/internal/git/file_reader_path_normalization_test.go b/internal/git/file_reader_path_normalization_test.go new file mode 100644 index 0000000..00b89b4 --- /dev/null +++ b/internal/git/file_reader_path_normalization_test.go @@ -0,0 +1,80 @@ +package git + +import ( + "testing" +) + +// TestGitRefFileReader_PathNormalization tests that absolute paths are correctly +// normalized to be relative to the repository root +func TestGitRefFileReader_PathNormalization(t *testing.T) { + tt := []struct { + name string + repoDir string + input string + expected string + }{ + { + name: "relative path unchanged", + repoDir: "/repo", + input: ".codeowners", + expected: ".codeowners", + }, + { + name: "absolute path stripped", + repoDir: "/repo", + input: "/repo/.codeowners", + expected: ".codeowners", + }, + { + name: "nested path with repo prefix", + repoDir: "/repo", + input: "/repo/internal/app/.codeowners", + expected: "internal/app/.codeowners", + }, + { + name: "current directory repo with relative path", + repoDir: ".", + input: "./.codeowners", + expected: ".codeowners", + }, + { + name: "github workspace path", + repoDir: "/github/workspace", + input: "/github/workspace/.codeowners", + expected: ".codeowners", + }, + { + name: "github workspace nested path", + repoDir: "/github/workspace", + input: "/github/workspace/internal/app/.codeowners", + expected: "internal/app/.codeowners", + }, + { + name: "path not under repo dir", + repoDir: "/repo", + input: "/other/.codeowners", + expected: "other/.codeowners", + }, + { + name: "repo dir without trailing slash", + repoDir: "/repo/", + input: "/repo/subdir/.codeowners", + expected: "subdir/.codeowners", + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + reader := &GitRefFileReader{ + ref: "test-ref", + dir: tc.repoDir, + } + + result := reader.normalizePathForGit(tc.input) + if result != tc.expected { + t.Errorf("normalizePathForGit(%q) with dir=%q = %q, want %q", + tc.input, tc.repoDir, result, tc.expected) + } + }) + } +} diff --git a/internal/git/file_reader_test.go b/internal/git/file_reader_test.go index 5bf5709..da82f10 100644 --- a/internal/git/file_reader_test.go +++ b/internal/git/file_reader_test.go @@ -68,6 +68,18 @@ func TestGitRefFileReader_ReadFile(t *testing.T) { expected: "* @owner1\n", expectError: false, }, + { + name: "read with absolute path including repo dir", + path: "/repo/.codeowners", + expected: "* @owner1\n", + expectError: false, + }, + { + name: "read subdirectory with absolute path", + path: "/repo/subdir/.codeowners", + expected: "*.js @owner2\n", + expectError: false, + }, } for _, tc := range tt { @@ -129,6 +141,11 @@ func TestGitRefFileReader_PathExists(t *testing.T) { path: "/.codeowners", expected: true, }, + { + name: "existing file with absolute path", + path: "/repo/.codeowners", + expected: true, + }, } for _, tc := range tt { From 20b4599ce54d89485256facf91bc4b49707a2a3d Mon Sep 17 00:00:00 2001 From: BakerNet Date: Thu, 30 Oct 2025 18:19:50 -0700 Subject: [PATCH 4/4] covbadge --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 3e56467..8ff95c5 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-81.7%25-brightgreen) +![Coverage](https://img.shields.io/badge/Coverage-81.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)