Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 63 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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-83.6%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)

Expand All @@ -23,6 +23,7 @@ Code Ownership & Review Assignment Tool - GitHub CODEOWNERS but better
- [Advanced Configuration](#advanced-configuration)
- [Enforcement Options](#enforcement-options)
- [Quiet Mode](#quiet-mode)
- [Hunk Filters](#hunk-filters)
- [CLI Tool](#cli-tool)
- [Contributing](#contributing)
- [Future Features](#future-features)
Expand All @@ -49,6 +50,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)
* Hunk filters: external tooling can decide which post-approval changes do not need re-review (see [Hunk Filters](#hunk-filters))
* Advanced global configuration (see [Advanced Configuration](#advanced-configuration))

## Getting Started
Expand Down Expand Up @@ -275,6 +277,10 @@ disable_review_status_comments = false
# `admin_bypass` allows repository administrators to bypass codeowner requirements
[admin_bypass]
# see "Admin Bypass" below for more details

# `hooks` names external programs the check may call
[hooks]
# see "Hunk Filters" below for more details
```

When a PR has any of the `high_priority_labels`, the comment will look like this:
Expand Down Expand Up @@ -386,6 +392,62 @@ 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.

### Hunk Filters

When an approval is checked, the diff is taken twice from the same merge base, once to the head and once to the approved commit, and the hunks appearing in both are subtracted. What is left is what the reviewer has not seen. That comparison is textual, so a hunk which changed for a reason a particular codebase does not care about still counts as unreviewed. Hunk filters let external tooling make that judgement and feed the answer back to Codeowners Plus.

Name an executable in the `[hooks]` table of `codeowners.toml`:

```toml
[hooks]
hunk_filter = "/opt/review-tools/already-seen"
```

Codeowners Plus reads `codeowners.toml` from the base ref, so a pull request cannot point the check at a program of its own.

For each approved commit being checked, the filter receives one JSON object on stdin:

```json
{
"version": 1,
"base": "<merge base sha>",
"head": "<head sha>",
"ref": "<the approved commit>",
"files": [
{
"name": "service/handler.go",
"head_hunks": [{ "body": "@@ -1,3 +1,3 @@\n-old\n+new\n" }],
"approval_hunks": [{ "body": "@@ -9,0 +10,1 @@\n+approved\n" }]
}
]
}
```

* `head_hunks`: the hunks still outstanding against this approval
* `approval_hunks`: the hunks the approved commit itself introduced, for comparison
* `ref`: the approved commit the head is being compared against
* `version`: the request format, currently `1`

It writes one JSON object on stdout, naming the `head_hunks` the reviewer has effectively already reviewed by index into the same file:

```json
{ "reviewed": [{ "name": "service/handler.go", "indexes": [0] }] }
```

Both sides of the diff are sent because they answer different questions: a hunk rewritten since approval looks like new code on its own, and only looks like reviewed code next to the text the reviewer saw.

Notes:

* A filter can only narrow what an approval answers for. It cannot add hunks, name a file it was not sent, alter ownership, or grant an approval; ownership resolution and the staleness check run unchanged on whatever survives.
* Every failure leaves the diff as Codeowners Plus computed it and logs a warning: the program is missing or not executable, it exits non-zero, it does not finish inside 60s, its output is not a single JSON object of the known shape, or it names a file or an index that was not sent.
* Unlike a configuration option, a filter is code in the review path, and it acts in the direction of dismissing less. It cannot weaken ownership, but it can decide that a change nobody looked at did not need looking at.
* The path must be absolute, and it is executed directly rather than through a shell. A relative path would resolve inside `GITHUB_WORKSPACE` and a bare name would go through `PATH`, both of which put the choice of program within reach of the pull request.
* A path under `GITHUB_WORKSPACE` is refused for the same reason: the checkout is writable by the PR author. Symlinks are followed before the check, so a link out of the checkout does not get around it. Install the program in the runner image, fetch it by digest, or build it from a pinned ref.
* Both of those refusals fail the run outright, unlike the failures above, which leave the diff alone and log a warning. A path that will not be run is a misconfiguration to fix, not a line in a log.
* The action's own inputs are removed from the filter's environment, `INPUT_GITHUB-TOKEN` among them, so a filter never sees the token the action runs with. The rest of the environment is inherited.
* Bounded per call: 60s, 8MB of stdout, 256KB of stderr into the run log. It runs once per distinct approved commit.
* A path in the same repository as the workflow is still a program someone can change. Treat it the way you would treat any other program in the review path.

## CLI Tool

A CLI tool is available which provides some utilities for working with `.codeowners` files.
Expand Down
82 changes: 81 additions & 1 deletion internal/app/app.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package app

import (
"context"
"fmt"
"io"
"path/filepath"
"slices"
"strings"
"time"
Expand All @@ -12,6 +14,7 @@ import (
gh "github.com/multimediallc/codeowners-plus/internal/github"
"github.com/multimediallc/codeowners-plus/pkg/codeowners"
f "github.com/multimediallc/codeowners-plus/pkg/functional"
"github.com/multimediallc/codeowners-plus/pkg/hook"
)

// OutputData holds the data that will be written to GITHUB_OUTPUT
Expand Down Expand Up @@ -58,10 +61,14 @@ type Config struct {
Repo string
Verbose bool
Quiet bool
Workspace string
InfoBuffer io.Writer
WarningBuffer io.Writer
}

// hunkFilterTimeout bounds each call, so a hook that hangs cannot hold up the check.
const hunkFilterTimeout = 60 * time.Second

// App represents the application with its dependencies
type App struct {
Conf *owners.Config
Expand Down Expand Up @@ -102,6 +109,70 @@ func (a *App) printWarn(format string, args ...interface{}) {
_, _ = fmt.Fprintf(a.config.WarningBuffer, format, args...)
}

// hunkFilter asks the hook which outstanding hunks are already reviewed; every failure is answered as "none".
func (a *App) hunkFilter(path, base, head string) git.HunkFilter {
return func(ref string, files []git.HunkText) (map[string][]int, error) {
req := hook.Request{Base: base, Head: head, Ref: ref}
for _, file := range files {
req.Files = append(req.Files, hook.File{
Name: file.Name,
HeadHunks: toHookHunks(file.HeadHunks),
ApprovalHunks: toHookHunks(file.ApprovalHunks),
})
}

ctx, cancel := context.WithTimeout(context.Background(), hunkFilterTimeout)
defer cancel()

res, err := hook.Run(ctx, path, req, a.config.WarningBuffer)
if err != nil {
a.printWarn("WARNING: hunk filter not applied: %v\n", err)
return nil, nil
}
reviewed, err := res.Indexes(req)
if err != nil {
a.printWarn("WARNING: hunk filter not applied: %v\n", err)
return nil, nil
}
return reviewed, nil
}
}

// A path that does not exist is cleaned, not refused, so a missing hook still reaches the run-time warning.
func resolveHookPath(path, workspace string) (string, error) {
if !filepath.IsAbs(path) {
return "", fmt.Errorf("%q is not an absolute path", path)
}
resolved := resolveSymlinks(path)
if workspace == "" {
return resolved, nil
}
root := resolveSymlinks(workspace)
rel, err := filepath.Rel(root, resolved)
if err != nil {
return resolved, nil
}
if rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
return "", fmt.Errorf("%q is inside the checkout at %s, which the pull request can write", path, workspace)
}
return resolved, nil
}

func resolveSymlinks(path string) string {
if real, err := filepath.EvalSymlinks(path); err == nil {
return real
}
return filepath.Clean(path)
}

func toHookHunks(bodies []string) []hook.Hunk {
hunks := make([]hook.Hunk, 0, len(bodies))
for _, body := range bodies {
hunks = append(hunks, hook.Hunk{Body: body})
}
return hunks
}

// Run executes the application logic
func (a *App) Run() (*OutputData, error) {
// Initialize PR
Expand Down Expand Up @@ -132,7 +203,16 @@ func (a *App) Run() (*OutputData, error) {

// Get the diff of the PR
a.printDebug("Getting diff for %s...%s\n", diffContext.Base, diffContext.Head)
gitDiff, err := git.NewDiff(diffContext)
var diffOpts []git.DiffOption
if conf.Hooks != nil && conf.Hooks.HunkFilter != "" {
hunkFilterPath, err := resolveHookPath(conf.Hooks.HunkFilter, a.config.Workspace)
if err != nil {
return &OutputData{}, fmt.Errorf("hooks.hunk_filter: %v", err)
}
a.printDebug("Using hunk filter %s\n", hunkFilterPath)
diffOpts = append(diffOpts, git.WithHunkFilter(a.hunkFilter(hunkFilterPath, diffContext.Base, diffContext.Head)))
}
gitDiff, err := git.NewDiff(diffContext, diffOpts...)
if err != nil {
return &OutputData{}, fmt.Errorf("NewGitDiff Error: %v", err)
}
Expand Down
Loading
Loading