diff --git a/README.md b/README.md index e3bcef7..75a8930 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-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) @@ -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) @@ -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 @@ -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: @@ -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": "", + "head": "", + "ref": "", + "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. diff --git a/internal/app/app.go b/internal/app/app.go index f7d364d..23589a6 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -1,8 +1,10 @@ package app import ( + "context" "fmt" "io" + "path/filepath" "slices" "strings" "time" @@ -12,6 +14,7 @@ import ( gh "github.com/multimediallc/codeowners-plus/internal/github" "github.com/multimediallc/codeowners-plus/pkg/codeowners" f "github.com/multimediallc/codeowners-plus/pkg/functional" + "github.com/multimediallc/codeowners-plus/pkg/hook" ) // OutputData holds the data that will be written to GITHUB_OUTPUT @@ -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 @@ -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 @@ -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) } diff --git a/internal/app/hunk_filter_test.go b/internal/app/hunk_filter_test.go new file mode 100644 index 0000000..c3c9c51 --- /dev/null +++ b/internal/app/hunk_filter_test.go @@ -0,0 +1,180 @@ +package app + +import ( + "bytes" + "os" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/multimediallc/codeowners-plus/internal/git" +) + +func writeFilterHook(t *testing.T, body string) string { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("shell hooks are not portable to windows") + } + path := filepath.Join(t.TempDir(), "filter") + if err := os.WriteFile(path, []byte("#!/bin/sh\n"+body), 0o755); err != nil { + t.Fatalf("writing hook: %v", err) + } + return path +} + +func filterFiles() []git.HunkText { + return []git.HunkText{{ + Name: "service.go", + HeadHunks: []string{"+one\n", "+two\n"}, + ApprovalHunks: []string{"+approved\n"}, + }} +} + +func TestAppHunkFilterAppliesAValidAnswer(t *testing.T) { + path := writeFilterHook(t, `cat >&2 +echo '{"reviewed":[{"name":"service.go","indexes":[1]}]}' +`) + warnings := &bytes.Buffer{} + a := &App{config: &Config{WarningBuffer: warnings, InfoBuffer: &bytes.Buffer{}}} + + reviewed, err := a.hunkFilter(path, "basesha", "headsha")("approvalsha", filterFiles()) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(reviewed["service.go"]) != 1 || reviewed["service.go"][0] != 1 { + t.Errorf("expected service.go index 1, got %v", reviewed) + } + + sent := warnings.String() + for _, want := range []string{`"ref":"approvalsha"`, `"base":"basesha"`, `"head":"headsha"`, `"approval_hunks"`, `"version":1`} { + if !strings.Contains(sent, want) { + t.Errorf("expected %s in the request, got %s", want, sent) + } + } +} + +func TestAppHunkFilterAnswersNoneOnFailure(t *testing.T) { + tt := []struct { + name string + body string + }{ + {"exits non-zero", "exit 1\n"}, + {"writes nothing", "true\n"}, + {"names an index that was not sent", `echo '{"reviewed":[{"name":"service.go","indexes":[7]}]}'` + "\n"}, + {"names a file that was not sent", `echo '{"reviewed":[{"name":"invented.go","indexes":[0]}]}'` + "\n"}, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + warnings := &bytes.Buffer{} + a := &App{config: &Config{ + WarningBuffer: warnings, + InfoBuffer: &bytes.Buffer{}, + }} + + reviewed, err := a.hunkFilter(writeFilterHook(t, tc.body), "basesha", "headsha")("approvalsha", filterFiles()) + if err != nil { + t.Errorf("expected the failure to be swallowed, got %v", err) + } + if len(reviewed) != 0 { + t.Errorf("expected nothing reviewed, got %v", reviewed) + } + if !strings.Contains(warnings.String(), "hunk filter not applied") { + t.Errorf("expected a warning, got %q", warnings.String()) + } + }) + } +} + +func TestAppHunkFilterMissingHookAnswersNone(t *testing.T) { + warnings := &bytes.Buffer{} + a := &App{config: &Config{ + WarningBuffer: warnings, + InfoBuffer: &bytes.Buffer{}, + }} + + reviewed, err := a.hunkFilter(filepath.Join(t.TempDir(), "absent"), "basesha", "headsha")("approvalsha", filterFiles()) + if err != nil { + t.Errorf("expected the failure to be swallowed, got %v", err) + } + if len(reviewed) != 0 { + t.Errorf("expected nothing reviewed, got %v", reviewed) + } + if !strings.Contains(warnings.String(), "hunk filter not applied") { + t.Errorf("expected a warning, got %q", warnings.String()) + } +} + +func TestResolveHookPathRejects(t *testing.T) { + workspace := t.TempDir() + outside := t.TempDir() + + tt := []struct { + name string + path string + want string + }{ + {"a relative path", "tools/filter", "not an absolute path"}, + {"a bare name off PATH", "filter", "not an absolute path"}, + {"a path in the checkout", filepath.Join(workspace, "tools", "filter"), "inside the checkout"}, + {"the checkout itself", workspace, "inside the checkout"}, + {"a traversal back into the checkout", filepath.Join(outside, "..", filepath.Base(workspace), "f"), "inside the checkout"}, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + if _, err := resolveHookPath(tc.path, workspace); err == nil { + t.Fatalf("expected %q to be refused", tc.path) + } else if !strings.Contains(err.Error(), tc.want) { + t.Errorf("expected %q in error, got %q", tc.want, err) + } + }) + } +} + +func TestResolveHookPathAllowsASiblingWithASharedPrefix(t *testing.T) { + root := t.TempDir() + workspace := filepath.Join(root, "work") + sibling := filepath.Join(root, "work-tools", "filter") + if err := os.MkdirAll(workspace, 0o755); err != nil { + t.Fatal(err) + } + + if _, err := resolveHookPath(sibling, workspace); err != nil { + t.Errorf("expected %q to be allowed, got %v", sibling, err) + } +} + +// A symlink planted in the checkout cannot carry a hook out of it on paper. +func TestResolveHookPathFollowsASymlinkIntoTheCheckout(t *testing.T) { + root := t.TempDir() + workspace := filepath.Join(root, "work") + if err := os.MkdirAll(workspace, 0o755); err != nil { + t.Fatal(err) + } + target := filepath.Join(workspace, "filter") + if err := os.WriteFile(target, []byte("#!/bin/sh\n"), 0o755); err != nil { + t.Fatal(err) + } + link := filepath.Join(root, "innocent") + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + if _, err := resolveHookPath(link, workspace); err == nil { + t.Error("expected a symlink into the checkout to be refused") + } +} + +// Local runs have no GITHUB_WORKSPACE, so an absolute path stands on its own. +func TestResolveHookPathWithoutAWorkspace(t *testing.T) { + path := filepath.Join(t.TempDir(), "filter") + got, err := resolveHookPath(path, "") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != filepath.Clean(path) { + t.Errorf("expected %q, got %q", path, got) + } +} diff --git a/internal/config/config.go b/internal/config/config.go index c3bcf7e..980a100 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -22,6 +22,11 @@ type Config struct { AllowSelfApproval bool `toml:"allow_self_approval"` SelfApprovalViaTeams bool `toml:"self_approval_via_teams"` DisableReviewStatusComments bool `toml:"disable_review_status_comments"` + Hooks *Hooks `toml:"hooks"` +} + +type Hooks struct { + HunkFilter string `toml:"hunk_filter"` } type Enforcement struct { @@ -52,6 +57,7 @@ func ReadConfig(path string, fileReader codeowners.FileReader) (*Config, error) DisableSmartDismissal: false, RequireBothBranchReviewers: false, DisableReviewStatusComments: false, + Hooks: &Hooks{}, } // Use filesystem reader if none provided @@ -79,5 +85,8 @@ func ReadConfig(path string, fileReader codeowners.FileReader) (*Config, error) if config.AdminBypass == nil { config.AdminBypass = defaultConfig.AdminBypass } + if config.Hooks == nil { + config.Hooks = &Hooks{} + } return config, nil } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index f09c117..921db8c 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -266,3 +266,35 @@ func sliceEqual(a, b []string) bool { } return true } + +func TestReadConfigHooks(t *testing.T) { + tt := []struct { + name string + content string + expected string + }{ + {"no hooks table", "min_reviews = 1\n", ""}, + {"empty hooks table", "[hooks]\n", ""}, + {"a hunk filter", "[hooks]\nhunk_filter = \"/opt/review-tools/already-seen\"\n", "/opt/review-tools/already-seen"}, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "codeowners.toml"), []byte(tc.content), 0o644); err != nil { + t.Fatal(err) + } + + conf, err := ReadConfig(dir, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if conf.Hooks == nil { + t.Fatal("expected Hooks to be non-nil so callers need no guard") + } + if conf.Hooks.HunkFilter != tc.expected { + t.Errorf("expected hunk_filter %q, got %q", tc.expected, conf.Hooks.HunkFilter) + } + }) + } +} diff --git a/internal/git/diff.go b/internal/git/diff.go index 95c74c0..b074498 100644 --- a/internal/git/diff.go +++ b/internal/git/diff.go @@ -40,18 +40,38 @@ type Diff interface { } type GitDiff struct { - context DiffContext - diff []*diff.FileDiff - files []codeowners.DiffFile - executor gitCommandExecutor + context DiffContext + diff []*diff.FileDiff + files []codeowners.DiffFile + executor gitCommandExecutor + hunkFilter HunkFilter } -func NewDiff(context DiffContext) (Diff, error) { +type HunkText struct { + Name string + HeadHunks []string + ApprovalHunks []string +} + +// HunkFilter reports already-reviewed hunks as indexes into each file's HeadHunks; an unrecognised file, an out-of-range index or any error leaves every hunk in place. +type HunkFilter func(ref string, files []HunkText) (map[string][]int, error) + +// DiffOption configures optional GitDiff behavior. +type DiffOption func(*GitDiff) + +// WithHunkFilter routes surviving hunks through filter before they reach an approval. +func WithHunkFilter(filter HunkFilter) DiffOption { + return func(gd *GitDiff) { + gd.hunkFilter = filter + } +} + +func NewDiff(context DiffContext, opts ...DiffOption) (Diff, error) { executor := newRealGitExecutor(context.Dir) - return NewDiffWithExecutor(context, executor) + return NewDiffWithExecutor(context, executor, opts...) } -func NewDiffWithExecutor(context DiffContext, executor gitCommandExecutor) (Diff, error) { +func NewDiffWithExecutor(context DiffContext, executor gitCommandExecutor, opts ...DiffOption) (Diff, error) { gitDiff, err := getGitDiff(context, executor) if err != nil { return nil, err @@ -61,12 +81,16 @@ func NewDiffWithExecutor(context DiffContext, executor gitCommandExecutor) (Diff return nil, err } - return &GitDiff{ + gd := &GitDiff{ context: context, diff: gitDiff, files: diffFiles, executor: executor, - }, nil + } + for _, opt := range opts { + opt(gd) + } + return gd, nil } func (gd *GitDiff) AllChanges() []codeowners.DiffFile { @@ -87,6 +111,8 @@ func (gd *GitDiff) ChangesSince(ref string) ([]codeowners.DiffFile, error) { changesContext := changesSinceContext{ newerDiff: gd.diff, olderDiff: olderDiff, + ref: ref, + filter: gd.hunkFilter, } diffFiles, err := changesSince(changesContext) if err != nil { @@ -109,6 +135,8 @@ type DiffContext struct { type changesSinceContext struct { newerDiff []*diff.FileDiff olderDiff []*diff.FileDiff + ref string + filter HunkFilter } func diffToFilename(d *diff.FileDiff) string { @@ -180,33 +208,152 @@ func changesSince(context changesSinceContext) ([]codeowners.DiffFile, error) { } } - diffFiles := make([]codeowners.DiffFile, 0, len(context.newerDiff)) + survivors := make([]survivingHunks, 0, len(context.newerDiff)) for _, d := range context.newerDiff { - fileName := diffToFilename(d) - - newDiffFile := codeowners.DiffFile{ - FileName: fileName, - Hunks: make([]codeowners.HunkRange, 0, len(d.Hunks)), - } + file := survivingHunks{name: diffToFilename(d)} for _, hunk := range d.Hunks { if !oldHunkHashes[hunkHash(hunk)] { - newHunkRange := codeowners.HunkRange{ - Start: int(hunk.NewStartLine), - End: int(hunk.NewStartLine + hunk.NewLines - 1), - } - newDiffFile.Hunks = append(newDiffFile.Hunks, newHunkRange) + file.hunks = append(file.hunks, survivingHunk{ + rng: codeowners.HunkRange{ + Start: int(hunk.NewStartLine), + End: int(hunk.NewStartLine + hunk.NewLines - 1), + }, + body: string(hunk.Body), + }) } } + survivors = append(survivors, file) + } + + if context.filter != nil { + survivors = applyHunkFilter(context, survivors) + } + + diffFiles := make([]codeowners.DiffFile, 0, len(survivors)) + for _, file := range survivors { // Binary files have no hunks; staleness is intentionally not tracked // for them (there is no hunk content to hash against the older diff). - if len(newDiffFile.Hunks) > 0 { - diffFiles = append(diffFiles, newDiffFile) + if len(file.hunks) == 0 { + continue } + diffFiles = append(diffFiles, codeowners.DiffFile{ + FileName: file.name, + Hunks: file.ranges(), + }) } return diffFiles, nil } +type survivingHunk struct { + rng codeowners.HunkRange + body string +} + +type survivingHunks struct { + name string + hunks []survivingHunk +} + +func (f survivingHunks) bodies() []string { + out := make([]string, 0, len(f.hunks)) + for _, h := range f.hunks { + out = append(out, h.body) + } + return out +} + +func (f survivingHunks) ranges() []codeowners.HunkRange { + out := make([]codeowners.HunkRange, 0, len(f.hunks)) + for _, h := range f.hunks { + out = append(out, h.rng) + } + return out +} + +// A path a filter cannot address is a path it must not be asked about: a +// typechange gives one path two entries and the answer is keyed by name. +func addressableNames(survivors []survivingHunks) map[string]bool { + count := make(map[string]int, len(survivors)) + for _, file := range survivors { + count[file.name]++ + } + names := make(map[string]bool, len(survivors)) + for _, file := range survivors { + if len(file.hunks) > 0 && count[file.name] == 1 { + names[file.name] = true + } + } + return names +} + +// applyHunkFilter drops the hunks the filter reports as already reviewed; any error, or any answer about something unasked, changes nothing. +func applyHunkFilter(context changesSinceContext, survivors []survivingHunks) []survivingHunks { + survivorNames := addressableNames(survivors) + if len(survivorNames) == 0 { + return survivors + } + + // Only the surviving files can be asked about, and olderDiff covers the whole PR. + approvalBodies := make(map[string][]string, len(survivorNames)) + for _, d := range context.olderDiff { + name := diffToFilename(d) + if !survivorNames[name] { + continue + } + for _, hunk := range d.Hunks { + approvalBodies[name] = append(approvalBodies[name], string(hunk.Body)) + } + } + + files := make([]HunkText, 0, len(survivorNames)) + for _, file := range survivors { + if !survivorNames[file.name] { + continue + } + files = append(files, HunkText{ + Name: file.name, + HeadHunks: file.bodies(), + ApprovalHunks: approvalBodies[file.name], + }) + } + + reviewed, err := context.filter(context.ref, files) + if err != nil || len(reviewed) == 0 { + return survivors + } + + filtered := make([]survivingHunks, 0, len(survivors)) + for _, file := range survivors { + indexes, ok := reviewed[file.name] + if !ok || len(indexes) == 0 || !survivorNames[file.name] { + filtered = append(filtered, file) + continue + } + drop := make(map[int]bool, len(indexes)) + for _, index := range indexes { + if index < 0 || index >= len(file.hunks) { + // An answer about an unsent hunk voids the answer for this file. + drop = nil + break + } + drop[index] = true + } + if drop == nil { + filtered = append(filtered, file) + continue + } + kept := survivingHunks{name: file.name} + for i, hunk := range file.hunks { + if !drop[i] { + kept.hunks = append(kept.hunks, hunk) + } + } + filtered = append(filtered, kept) + } + return filtered +} + func getGitDiff(data DiffContext, executor gitCommandExecutor) ([]*diff.FileDiff, error) { cmdOutput, err := executor.execute("git", "diff", "-U0", fmt.Sprintf("%s...%s", data.Base, data.Head)) if err != nil { diff --git a/internal/git/diff_test.go b/internal/git/diff_test.go index 59a2244..4a93dad 100644 --- a/internal/git/diff_test.go +++ b/internal/git/diff_test.go @@ -770,7 +770,7 @@ func TestDiffOfDiffs(t *testing.T) { t.Errorf("Error parsing diff changes: %v", err) } - diffOutput, err := changesSince(changesSinceContext{newDiff, oldDiff}) + diffOutput, err := changesSince(changesSinceContext{newerDiff: newDiff, olderDiff: oldDiff}) if err != nil { t.Errorf("Error getting diff of diffs: %v", err) } diff --git a/internal/git/hunkfilter_test.go b/internal/git/hunkfilter_test.go new file mode 100644 index 0000000..78f90a3 --- /dev/null +++ b/internal/git/hunkfilter_test.go @@ -0,0 +1,209 @@ +package git + +import ( + "errors" + "testing" + + "github.com/sourcegraph/go-diff/diff" +) + +const filterHeadDiff = `diff --git a/service.go b/service.go +--- a/service.go ++++ b/service.go +@@ -1,0 +2,1 @@ ++// a note the reviewer has seen +@@ -10,0 +12,1 @@ ++realChange() +diff --git a/other.go b/other.go +--- a/other.go ++++ b/other.go +@@ -3,0 +4,1 @@ ++untouchedByTheFilter() +` + +const filterApprovalDiff = `diff --git a/service.go b/service.go +--- a/service.go ++++ b/service.go +@@ -20,0 +21,1 @@ ++approvedEarlier() +` + +func parseFilterDiff(t *testing.T, body string) []*diff.FileDiff { + t.Helper() + parsed, err := diff.ParseMultiFileDiff([]byte(body)) + if err != nil { + t.Fatalf("parsing diff: %v", err) + } + return parsed +} + +func hunkCounts(t *testing.T, filter HunkFilter) map[string]int { + t.Helper() + files, err := changesSince(changesSinceContext{ + newerDiff: parseFilterDiff(t, filterHeadDiff), + olderDiff: parseFilterDiff(t, filterApprovalDiff), + ref: "approvalsha", + filter: filter, + }) + if err != nil { + t.Fatalf("changesSince: %v", err) + } + counts := make(map[string]int, len(files)) + for _, file := range files { + counts[file.FileName] = len(file.Hunks) + } + return counts +} + +func TestHunkFilterSkipsDuplicateFilenames(t *testing.T) { + const dupDiff = `diff --git a/x b/x +--- a/x ++++ /dev/null +@@ -1,1 +0,0 @@ +-was a file +diff --git a/x b/x +--- /dev/null ++++ b/x +@@ -0,0 +1,1 @@ ++/etc/hostname +` + var offered []HunkText + files, err := changesSince(changesSinceContext{ + newerDiff: parseFilterDiff(t, dupDiff), + olderDiff: nil, + ref: "approvalsha", + filter: func(_ string, f []HunkText) (map[string][]int, error) { + offered = f + return map[string][]int{"x": {0}}, nil + }, + }) + if err != nil { + t.Fatalf("changesSince: %v", err) + } + for _, f := range offered { + if f.Name == "x" { + t.Errorf("a duplicate-named path must not be offered to the filter, got %+v", f) + } + } + total := 0 + for _, f := range files { + if f.FileName == "x" { + total += len(f.Hunks) + } + } + if total != 2 { + t.Errorf("expected both hunks for x to survive, got %d", total) + } +} + +func TestChangesSinceWithoutFilterIsUnchanged(t *testing.T) { + counts := hunkCounts(t, nil) + if counts["service.go"] != 2 || counts["other.go"] != 1 { + t.Errorf("expected service.go:2 other.go:1, got %v", counts) + } +} + +func TestHunkFilterDropsOnlyWhatItNames(t *testing.T) { + var gotRef string + var gotApproval []string + counts := hunkCounts(t, func(ref string, files []HunkText) (map[string][]int, error) { + gotRef = ref + for _, file := range files { + if file.Name == "service.go" { + gotApproval = file.ApprovalHunks + } + } + return map[string][]int{"service.go": {0}}, nil + }) + + if gotRef != "approvalsha" { + t.Errorf("expected the approval ref to be passed, got %q", gotRef) + } + if len(gotApproval) != 1 { + t.Errorf("expected service.go to carry 1 approval hunk, got %d", len(gotApproval)) + } + if counts["service.go"] != 1 { + t.Errorf("expected 1 hunk left in service.go, got %d", counts["service.go"]) + } + if counts["other.go"] != 1 { + t.Errorf("expected other.go untouched, got %d", counts["other.go"]) + } +} + +func TestHunkFilterDroppingEveryHunkRemovesTheFile(t *testing.T) { + counts := hunkCounts(t, func(string, []HunkText) (map[string][]int, error) { + return map[string][]int{"service.go": {0, 1}}, nil + }) + if _, present := counts["service.go"]; present { + t.Errorf("expected service.go to drop out entirely, got %v", counts) + } + if counts["other.go"] != 1 { + t.Errorf("expected other.go untouched, got %d", counts["other.go"]) + } +} + +func TestHunkFilterFailuresChangeNothing(t *testing.T) { + tt := []struct { + name string + filter HunkFilter + }{ + { + name: "error", + filter: func(string, []HunkText) (map[string][]int, error) { + return map[string][]int{"service.go": {0}}, errors.New("hook exploded") + }, + }, + { + name: "index past the end", + filter: func(string, []HunkText) (map[string][]int, error) { + return map[string][]int{"service.go": {0, 9}}, nil + }, + }, + { + name: "negative index", + filter: func(string, []HunkText) (map[string][]int, error) { + return map[string][]int{"service.go": {0, -1}}, nil + }, + }, + { + name: "file that was never sent", + filter: func(string, []HunkText) (map[string][]int, error) { + return map[string][]int{"invented.go": {0}}, nil + }, + }, + { + name: "nothing named", + filter: func(string, []HunkText) (map[string][]int, error) { + return nil, nil + }, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + counts := hunkCounts(t, tc.filter) + if counts["service.go"] != 2 || counts["other.go"] != 1 { + t.Errorf("expected service.go:2 other.go:1, got %v", counts) + } + }) + } +} + +func TestHunkFilterOnlySeesSurvivingHunks(t *testing.T) { + var sent []HunkText + _, err := changesSince(changesSinceContext{ + newerDiff: parseFilterDiff(t, filterHeadDiff), + olderDiff: parseFilterDiff(t, filterHeadDiff), + ref: "approvalsha", + filter: func(_ string, files []HunkText) (map[string][]int, error) { + sent = files + return nil, nil + }, + }) + if err != nil { + t.Fatalf("changesSince: %v", err) + } + if len(sent) != 0 { + t.Errorf("expected the filter not to be called with nothing outstanding, got %v", sent) + } +} diff --git a/main.go b/main.go index 74a0883..636cd7d 100644 --- a/main.go +++ b/main.go @@ -16,13 +16,14 @@ import ( // Flags holds the command line flags type Flags struct { - Token *string - ApiUrl *string - RepoDir *string - PR *int - Repo *string - Verbose *bool - Quiet *bool + Token *string + ApiUrl *string + RepoDir *string + PR *int + Repo *string + Verbose *bool + Quiet *bool + Workspace *string } var ( @@ -38,6 +39,8 @@ var ( 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"), + // -dir cannot stand in for this: it falls back to "/", which would put every absolute path inside the checkout. + Workspace: flag.String("workspace", getEnv("GITHUB_WORKSPACE", ""), "Path to the checkout, which hook paths may not live under"), } WarningBuffer = bytes.NewBuffer([]byte{}) InfoBuffer = bytes.NewBuffer([]byte{}) @@ -151,6 +154,7 @@ func main() { Repo: *flags.Repo, Verbose: *flags.Verbose, Quiet: *flags.Quiet, + Workspace: *flags.Workspace, InfoBuffer: InfoBuffer, WarningBuffer: WarningBuffer, } diff --git a/pkg/hook/hook.go b/pkg/hook/hook.go new file mode 100644 index 0000000..593f76e --- /dev/null +++ b/pkg/hook/hook.go @@ -0,0 +1,180 @@ +// Package hook asks an external program which post-approval hunks are already reviewed (see "Hunk Filters" in the README). +package hook + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "os" + "os/exec" + "path/filepath" + "strings" + "time" +) + +// RequestVersion is bumped when Request changes shape in a way a hook could misread. +const RequestVersion = 1 + +const maxResponseBytes = 8 << 20 + +// Killing a hook does not close pipes its own children inherited, so Wait would +// block on a surviving grandchild for as long as it lived. +const ioGrace = 2 * time.Second + +// maxStderrBytes bounds what a hook can add to the run log. +const maxStderrBytes = 256 << 10 + +// cappedWriter forwards at most limit bytes and drops the rest, remembering that +// it had to. Every write is reported as complete so the hook never sees a short +// write it cannot act on. +type cappedWriter struct { + to io.Writer + limit int + written int + overflow bool +} + +func (c *cappedWriter) Write(p []byte) (int, error) { + room := c.limit - c.written + if len(p) > room { + c.overflow = true + } else { + room = len(p) + } + if room > 0 { + n, err := c.to.Write(p[:room]) + c.written += n + if err != nil { + return len(p), nil + } + } + return len(p), nil +} + +func withoutActionInputs(env []string) []string { + out := make([]string, 0, len(env)) + for _, kv := range env { + if strings.HasPrefix(kv, "INPUT_") { + continue + } + out = append(out, kv) + } + return out +} + +type Hunk struct { + Body string `json:"body"` +} + +type File struct { + Name string `json:"name"` + HeadHunks []Hunk `json:"head_hunks"` + ApprovalHunks []Hunk `json:"approval_hunks"` +} + +type Request struct { + Version int `json:"version"` + Base string `json:"base"` + Head string `json:"head"` + Ref string `json:"ref"` + Files []File `json:"files"` +} + +// ReviewedFile names hunks by index into the same file's head_hunks in the request. +type ReviewedFile struct { + Name string `json:"name"` + Indexes []int `json:"indexes"` +} + +// Anything not named in a Response stays in the diff. +type Response struct { + Reviewed []ReviewedFile `json:"reviewed"` +} + +// Indexes validates a response against the request it answers; an unsent name or out-of-range index fails the whole answer rather than being skipped. +func (r Response) Indexes(req Request) (map[string][]int, error) { + sent := make(map[string]int, len(req.Files)) + for _, file := range req.Files { + sent[file.Name] = len(file.HeadHunks) + } + + reviewed := make(map[string][]int, len(r.Reviewed)) + for _, file := range r.Reviewed { + count, ok := sent[file.Name] + if !ok { + return nil, fmt.Errorf("hook named file %q, which was not sent", file.Name) + } + if _, seen := reviewed[file.Name]; seen { + return nil, fmt.Errorf("hook named file %q twice", file.Name) + } + seenIndex := make(map[int]bool, len(file.Indexes)) + indexes := make([]int, 0, len(file.Indexes)) + for _, index := range file.Indexes { + if index < 0 || index >= count { + return nil, fmt.Errorf("hook named index %d for %q, which has %d hunks", index, file.Name, count) + } + if seenIndex[index] { + continue + } + seenIndex[index] = true + indexes = append(indexes, index) + } + reviewed[file.Name] = indexes + } + return reviewed, nil +} + +// Decoder.More reports false on a stray closing delimiter, so it cannot tell a +// clean end of stream from trailing junk; only another Decode can. +func atEndOfStream(d *json.Decoder) bool { + var trailing json.RawMessage + return d.Decode(&trailing) == io.EOF +} + +// Run writes req to the hook's stdin and decodes a Response from its stdout; the path is executed directly, never through a shell. +func Run(ctx context.Context, path string, req Request, stderr io.Writer) (Response, error) { + if !filepath.IsAbs(path) { + return Response{}, fmt.Errorf("hook path %q must be absolute", path) + } + + req.Version = RequestVersion + + input, err := json.Marshal(req) + if err != nil { + return Response{}, fmt.Errorf("encoding hook request: %w", err) + } + + var body bytes.Buffer + stdout := &cappedWriter{to: &body, limit: maxResponseBytes} + cmd := exec.CommandContext(ctx, path) + cmd.Stdin = bytes.NewReader(input) + cmd.Stdout = stdout + cmd.Stderr = &cappedWriter{to: stderr, limit: maxStderrBytes} + cmd.Env = withoutActionInputs(os.Environ()) + cmd.WaitDelay = ioGrace + + if err := cmd.Run(); err != nil { + if ctxErr := ctx.Err(); ctxErr != nil { + return Response{}, fmt.Errorf("hook %s did not finish: %w", path, ctxErr) + } + return Response{}, fmt.Errorf("hook %s failed: %w", path, err) + } + if stdout.overflow { + return Response{}, fmt.Errorf("hook %s wrote more than the %d byte limit", path, maxResponseBytes) + } + + decoder := json.NewDecoder(&body) + // An unknown field means the hook may believe it answered when it did not. + decoder.DisallowUnknownFields() + + var res Response + if err := decoder.Decode(&res); err != nil { + return Response{}, fmt.Errorf("decoding hook response: %w", err) + } + if !atEndOfStream(decoder) { + return Response{}, fmt.Errorf("hook %s wrote more than one JSON value", path) + } + return res, nil +} diff --git a/pkg/hook/hook_test.go b/pkg/hook/hook_test.go new file mode 100644 index 0000000..21f934a --- /dev/null +++ b/pkg/hook/hook_test.go @@ -0,0 +1,201 @@ +package hook + +import ( + "context" + "io" + "os" + "path/filepath" + "runtime" + "slices" + "strings" + "testing" + "time" +) + +func sampleRequest() Request { + return Request{ + Base: "basesha", + Head: "headsha", + Ref: "approvalsha", + Files: []File{ + {Name: "a.go", HeadHunks: []Hunk{{Body: "+one\n"}, {Body: "+two\n"}}}, + {Name: "b.go", HeadHunks: []Hunk{{Body: "+three\n"}}}, + }, + } +} + +func TestIndexesAcceptsWhatWasSent(t *testing.T) { + req := sampleRequest() + res := Response{Reviewed: []ReviewedFile{ + {Name: "a.go", Indexes: []int{1, 1, 0}}, + {Name: "b.go", Indexes: []int{}}, + }} + + reviewed, err := res.Indexes(req) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !slices.Equal(reviewed["a.go"], []int{1, 0}) { + t.Errorf("expected a repeated index to collapse and keep its order, got %v", reviewed["a.go"]) + } + if len(reviewed["b.go"]) != 0 { + t.Errorf("expected b.go to name nothing, got %v", reviewed["b.go"]) + } +} + +func TestIndexesRejectsAnswersOutsideTheRequest(t *testing.T) { + tt := []struct { + name string + res Response + }{ + {"file never sent", Response{Reviewed: []ReviewedFile{{Name: "invented.go", Indexes: []int{0}}}}}, + {"index past the end", Response{Reviewed: []ReviewedFile{{Name: "b.go", Indexes: []int{1}}}}}, + {"negative index", Response{Reviewed: []ReviewedFile{{Name: "a.go", Indexes: []int{-1}}}}}, + {"file named twice", Response{Reviewed: []ReviewedFile{ + {Name: "a.go", Indexes: []int{0}}, + {Name: "a.go", Indexes: []int{1}}, + }}}, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + if _, err := tc.res.Indexes(sampleRequest()); err == nil { + t.Error("expected an error") + } + }) + } +} + +func writeHook(t *testing.T, body string) string { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("shell hooks are not portable to windows") + } + path := filepath.Join(t.TempDir(), "hook") + if err := os.WriteFile(path, []byte("#!/bin/sh\n"+body), 0o755); err != nil { + t.Fatalf("writing hook: %v", err) + } + return path +} + +func TestRunReadsTheResponse(t *testing.T) { + path := writeHook(t, `cat >&2 +echo '{"reviewed":[{"name":"a.go","indexes":[0]}]}' +`) + var stderr strings.Builder + res, err := Run(context.Background(), path, sampleRequest(), &stderr) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(res.Reviewed) != 1 || res.Reviewed[0].Name != "a.go" { + t.Fatalf("unexpected response: %+v", res.Reviewed) + } + + sent := stderr.String() + if !strings.Contains(sent, `"version":1`) { + t.Errorf("expected the contract version to be sent, got %s", sent) + } + if !strings.Contains(sent, `"ref":"approvalsha"`) { + t.Errorf("expected the approval ref to be sent, got %s", sent) + } + if !strings.Contains(sent, `"approval_hunks"`) { + t.Errorf("expected the approval-time hunks to be sent, got %s", sent) + } +} + +func TestRunFailsLoudly(t *testing.T) { + tt := []struct { + name string + body string + }{ + {"non-zero exit", "exit 1\n"}, + {"no output", "true\n"}, + {"not json", "echo not json\n"}, + {"unknown field", `echo '{"reviewed":[],"verdict":"yes"}'` + "\n"}, + {"two values", `echo '{"reviewed":[]} {"reviewed":[]}'` + "\n"}, + {"trailing brace", `echo '{"reviewed":[]}}'` + "\n"}, + {"trailing bracket", `echo '{"reviewed":[]}]'` + "\n"}, + {"trailing junk", `echo '{"reviewed":[]} nope'` + "\n"}, + {"over the size limit", "head -c 9000000 /dev/zero | tr '\\0' 'x'\n"}, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + path := writeHook(t, tc.body) + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + if _, err := Run(ctx, path, sampleRequest(), io.Discard); err == nil { + t.Error("expected an error") + } + }) + } +} + +func TestRunRejectsARelativePath(t *testing.T) { + for _, path := range []string{"filter", "./tools/filter", "tools/filter"} { + if _, err := Run(context.Background(), path, sampleRequest(), io.Discard); err == nil { + t.Errorf("expected %q to be rejected", path) + } + } +} + +func TestRunDoesNotHandTheHookTheActionInputs(t *testing.T) { + path := writeHook(t, `env >&2 +echo '{"reviewed":[]}' +`) + t.Setenv("INPUT_GITHUB-TOKEN", "ghs_secretvalue") + t.Setenv("INPUT_PR", "1") + t.Setenv("KEPT_FOR_THE_HOOK", "yes") + + var stderr strings.Builder + if _, err := Run(context.Background(), path, sampleRequest(), &stderr); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if strings.Contains(stderr.String(), "ghs_secretvalue") { + t.Error("the hook must not receive the action's token") + } + if strings.Contains(stderr.String(), "INPUT_PR") { + t.Error("the hook must not receive the action's inputs") + } + if !strings.Contains(stderr.String(), "KEPT_FOR_THE_HOOK") { + t.Error("the rest of the environment should still reach the hook") + } +} + +func TestRunBoundsHookStderr(t *testing.T) { + path := writeHook(t, `head -c 2000000 /dev/zero | tr '\0' 'e' >&2 +echo '{"reviewed":[]}' +`) + var stderr strings.Builder + if _, err := Run(context.Background(), path, sampleRequest(), &stderr); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if stderr.Len() > maxStderrBytes { + t.Errorf("expected stderr capped at %d bytes, got %d", maxStderrBytes, stderr.Len()) + } +} + +func TestRunMissingHookIsAnError(t *testing.T) { + missing := filepath.Join(t.TempDir(), "absent") + if _, err := Run(context.Background(), missing, sampleRequest(), io.Discard); err == nil { + t.Error("expected an error for a hook that does not exist") + } +} + +func TestRunHonoursTheDeadline(t *testing.T) { + path := writeHook(t, "sleep 30\n") + ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) + defer cancel() + + start := time.Now() + _, err := Run(ctx, path, sampleRequest(), io.Discard) + if err == nil { + t.Fatal("expected an error") + } + if elapsed := time.Since(start); elapsed > 10*time.Second { + t.Errorf("expected the deadline to cut the hook off, took %s", elapsed) + } + if !strings.Contains(err.Error(), "did not finish") { + t.Errorf("expected the deadline to be named as the cause, got %v", err) + } +}