From c84c48fdd3c7310501a1f902544988454cda6b93 Mon Sep 17 00:00:00 2001 From: Ezekiel Lopez Date: Tue, 1 Sep 2026 00:09:37 -0700 Subject: [PATCH 1/4] feat: hunk filters (externally decided already-reviewed hunks) 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 hunks appearing in both are subtracted. That subtraction is exact but literal: a hunk which has been reformatted, or had a comment edited, or is a generated file being regenerated, is not the same text and so counts as unreviewed. Whether that is right depends on the codebase. In one repository a reformat is noise; in another the formatter changing its mind is the thing somebody needs to look at. This action should not pick one on a repository's behalf, and every consumer would inherit whichever it picked. Add a seam instead. The new `hunk-filter` input names a program which receives the hunks still outstanding against an approval, alongside the hunks that approval introduced, and answers which of them the reviewer has effectively already reviewed. The policy lives with whoever wants it. Design properties: - Subtractive only: a filter can narrow what an approval is held responsible for and nothing else. It cannot add hunks, name a file it was not sent, touch ownership, or grant an approval. Resolution and the staleness check downstream run unaltered on what survives. - Fail-open on the safe side: a missing or non-executable program, a non-zero exit, a timeout, output that is not a single known-shaped JSON object, or an index outside what was sent all leave the diff exactly as computed, with a warning. A broken filter costs the optimisation, never the gate. - Bounded: 60s per call, 8MB of output, executed directly rather than through a shell, once per distinct approved commit. The README is explicit that this is a trust decision, and that a path inside the checkout is writable by the pull request author and so is the wrong thing to name. Code changes: - `pkg/hook`: wire contract, subprocess runner, response validation - `internal/git`: `DiffOption`, `WithHunkFilter`, filtering in `changesSince` - `internal/app`: hook wiring and the fail-safe policy; `main.go`/`action.yml`: `hunk-filter` input - README: "Hunk Filters" section - Tests: contract validation, every runner failure mode, and that each way a filter can misbehave leaves the diff untouched Coverage badge regenerated. --- README.md | 58 ++++++++++- action.yml | 5 + internal/app/app.go | 50 ++++++++- internal/app/hunk_filter_test.go | 109 ++++++++++++++++++++ internal/git/diff.go | 159 +++++++++++++++++++++++++---- internal/git/diff_test.go | 2 +- internal/git/hunkfilter_test.go | 168 +++++++++++++++++++++++++++++++ main.go | 16 +-- pkg/hook/hook.go | 141 ++++++++++++++++++++++++++ pkg/hook/hook_test.go | 154 ++++++++++++++++++++++++++++ 10 files changed, 832 insertions(+), 30 deletions(-) create mode 100644 internal/app/hunk_filter_test.go create mode 100644 internal/git/hunkfilter_test.go create mode 100644 pkg/hook/hook.go create mode 100644 pkg/hook/hook_test.go diff --git a/README.md b/README.md index 90857ad..06ceff2 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ Code Ownership & Review Assignment Tool - GitHub CODEOWNERS but better [![Go Report Card](https://goreportcard.com/badge/github.com/multimediallc/codeowners-plus)](https://goreportcard.com/report/github.com/multimediallc/codeowners-plus?kill_cache=1) [![Tests](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml/badge.svg)](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml) -![Coverage](https://img.shields.io/badge/Coverage-82.6%25-brightgreen) +![Coverage](https://img.shields.io/badge/Coverage-83.3%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) + - [Hunk Filters](#hunk-filters) - [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) +* 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 @@ -372,6 +374,60 @@ 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. + +Pass an executable to the action with the `hunk-filter` input: + +```yaml + - name: 'Codeowners Plus' + uses: multimediallc/codeowners-plus@v1 + with: + github-token: '${{ secrets.GITHUB_TOKEN }}' + pr: '${{ github.event.pull_request.number }}' + hunk-filter: '/opt/review-tools/already-seen' +``` + +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 workflow naming the filter comes from the base branch, but a path inside `GITHUB_WORKSPACE` does not: the checkout is writable by the PR author, so a filter named there lets a contributor choose the code that judges their own changes. Install it in the runner image, fetch it by digest, or build it from a pinned ref. +* The program is executed directly rather than through a shell, once per distinct approved commit, bounded at 60s and 8MB of output per call. + ## CLI Tool A CLI tool is available which provides some utilities for working with `.codeowners` files. diff --git a/action.yml b/action.yml index bc63b95..f5800e5 100644 --- a/action.yml +++ b/action.yml @@ -23,6 +23,10 @@ inputs: description: 'Disable PR comments and review requests' required: false default: false + hunk-filter: + description: 'Path to an executable which reports already-reviewed post-approval hunks; see "Hunk Filters" in the README' + required: false + default: '' outputs: data: @@ -113,5 +117,6 @@ runs: INPUT_REPOSITORY: ${{ inputs.repository }} INPUT_VERBOSE: ${{ inputs.verbose }} INPUT_QUIET: ${{ inputs.quiet }} + INPUT_HUNK-FILTER: ${{ inputs.hunk-filter }} BIN: ${{ steps.resolve.outputs.bin }} run: '"${BIN}"' diff --git a/internal/app/app.go b/internal/app/app.go index d962024..b9501a6 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -1,6 +1,7 @@ package app import ( + "context" "fmt" "io" "slices" @@ -12,6 +13,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 @@ -57,10 +59,14 @@ type Config struct { Repo string Verbose bool Quiet bool + HunkFilter 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 @@ -101,6 +107,43 @@ 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(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, a.config.HunkFilter, 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 + } +} + +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 @@ -131,7 +174,12 @@ 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 a.config.HunkFilter != "" { + a.printDebug("Using hunk filter %s\n", a.config.HunkFilter) + diffOpts = append(diffOpts, git.WithHunkFilter(a.hunkFilter(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..76ee8c5 --- /dev/null +++ b/internal/app/hunk_filter_test.go @@ -0,0 +1,109 @@ +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{HunkFilter: path, WarningBuffer: warnings, InfoBuffer: &bytes.Buffer{}}} + + reviewed, err := a.hunkFilter("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{ + HunkFilter: writeFilterHook(t, tc.body), + WarningBuffer: warnings, + InfoBuffer: &bytes.Buffer{}, + }} + + reviewed, err := a.hunkFilter("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{ + HunkFilter: filepath.Join(t.TempDir(), "absent"), + WarningBuffer: warnings, + InfoBuffer: &bytes.Buffer{}, + }} + + reviewed, err := a.hunkFilter("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()) + } +} diff --git a/internal/git/diff.go b/internal/git/diff.go index 95c74c0..de9c630 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,122 @@ 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{ + file.ranges = append(file.ranges, codeowners.HunkRange{ Start: int(hunk.NewStartLine), End: int(hunk.NewStartLine + hunk.NewLines - 1), - } - newDiffFile.Hunks = append(newDiffFile.Hunks, newHunkRange) + }) + file.bodies = append(file.bodies, 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.ranges) == 0 { + continue } + diffFiles = append(diffFiles, codeowners.DiffFile{ + FileName: file.name, + Hunks: file.ranges, + }) } return diffFiles, nil } +// Each range keeps the text it came from, so a filter can be asked about it. +type survivingHunks struct { + name string + ranges []codeowners.HunkRange + bodies []string +} + +// 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 := make(map[string]bool, len(survivors)) + for _, file := range survivors { + if len(file.bodies) > 0 { + survivorNames[file.name] = true + } + } + 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 len(file.bodies) == 0 { + 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 { + filtered = append(filtered, file) + continue + } + drop := make(map[int]bool, len(indexes)) + for _, index := range indexes { + if index < 0 || index >= len(file.ranges) { + // Answering about unsent hunks voids its 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 := range file.ranges { + if drop[i] { + continue + } + kept.ranges = append(kept.ranges, file.ranges[i]) + kept.bodies = append(kept.bodies, file.bodies[i]) + } + 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..4e7d5d5 --- /dev/null +++ b/internal/git/hunkfilter_test.go @@ -0,0 +1,168 @@ +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 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": {-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 ecd3d9f..8c6235e 100644 --- a/main.go +++ b/main.go @@ -15,12 +15,13 @@ 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 + Verbose *bool + Quiet *bool + HunkFilter *string } var ( @@ -31,6 +32,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"), + HunkFilter: flag.String("hunk-filter", getEnv("INPUT_HUNK-FILTER", ""), + "Path to an executable which reports already-reviewed hunks (see README)"), } WarningBuffer = bytes.NewBuffer([]byte{}) InfoBuffer = bytes.NewBuffer([]byte{}) @@ -132,6 +135,7 @@ func main() { Repo: *flags.Repo, Verbose: *flags.Verbose, Quiet: *flags.Quiet, + HunkFilter: *flags.HunkFilter, InfoBuffer: InfoBuffer, WarningBuffer: WarningBuffer, } diff --git a/pkg/hook/hook.go b/pkg/hook/hook.go new file mode 100644 index 0000000..ce36db5 --- /dev/null +++ b/pkg/hook/hook.go @@ -0,0 +1,141 @@ +// 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/exec" + "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 + +// limitedBuffer keeps at most limit bytes, reporting every write as complete so the hook does not die on a short write. +type limitedBuffer struct { + buf bytes.Buffer + limit int + overflow bool +} + +func (b *limitedBuffer) Write(p []byte) (int, error) { + if room := b.limit - b.buf.Len(); len(p) > room { + b.overflow = true + if room > 0 { + b.buf.Write(p[:room]) + } + return len(p), nil + } + return b.buf.Write(p) +} + +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 +} + +// 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) { + req.Version = RequestVersion + + input, err := json.Marshal(req) + if err != nil { + return Response{}, fmt.Errorf("encoding hook request: %w", err) + } + + stdout := &limitedBuffer{limit: maxResponseBytes} + cmd := exec.CommandContext(ctx, path) + cmd.Stdin = bytes.NewReader(input) + cmd.Stdout = stdout + cmd.Stderr = stderr + 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(&stdout.buf) + // 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 decoder.More() { + 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..74bcd6e --- /dev/null +++ b/pkg/hook/hook_test.go @@ -0,0 +1,154 @@ +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"}, + {"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 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) + } +} From 6b7619d7a89528b607c2551f96dc9175ef2318a4 Mon Sep 17 00:00:00 2001 From: Ezekiel Lopez Date: Tue, 1 Sep 2026 12:30:12 -0700 Subject: [PATCH 2/4] fix: reject a hook response with trailing JSON Review catch: json.Decoder.More() reports false when the next byte is a closing delimiter, so it cannot tell a clean end of stream from trailing junk. A response like {"reviewed":[...]}} passed validation and its indexes were applied. Accepting a malformed response breaks the guarantee that anything unexpected leaves the diff whole, and it fails in the dismiss-less direction. Require a second Decode to return io.EOF instead, and cover a trailing brace, bracket and bare token. --- README.md | 2 +- pkg/hook/hook.go | 5 ++++- pkg/hook/hook_test.go | 3 +++ 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 06ceff2..efd2de5 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-83.3%25-brightgreen) +![Coverage](https://img.shields.io/badge/Coverage-83.4%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/pkg/hook/hook.go b/pkg/hook/hook.go index ce36db5..11d7644 100644 --- a/pkg/hook/hook.go +++ b/pkg/hook/hook.go @@ -134,7 +134,10 @@ func Run(ctx context.Context, path string, req Request, stderr io.Writer) (Respo if err := decoder.Decode(&res); err != nil { return Response{}, fmt.Errorf("decoding hook response: %w", err) } - if decoder.More() { + // More() reports false on a stray closing delimiter, so it cannot tell a clean + // end of stream from trailing junk. Only a second Decode returning EOF can. + var trailing json.RawMessage + if err := decoder.Decode(&trailing); err != io.EOF { 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 index 74bcd6e..af7ec6b 100644 --- a/pkg/hook/hook_test.go +++ b/pkg/hook/hook_test.go @@ -113,6 +113,9 @@ func TestRunFailsLoudly(t *testing.T) { {"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"}, } From 6fe780eb24003c67878f3fd8f4b030ab5492823f Mon Sep 17 00:00:00 2001 From: Ezekiel Lopez Date: Tue, 1 Sep 2026 14:00:44 -0700 Subject: [PATCH 3/4] fix: close four ways a filter could reach past what it was asked Review catches, in order of how much they mattered. A path can appear twice in one diff. git emits a delete and an add for the same name when its type changes, file to symlink among others, and the answer is keyed by name, so a filter naming that path spoke for both entries. Answering one index for one file dropped two hunks, and the path left the approval diff entirely, so an unreviewed deletion went unnoticed. The protocol has no way to say which entry is meant, so a duplicate-named path is no longer offered to the filter at all, and an answer naming one is ignored. stderr was unbounded while stdout was capped, which made the documented "60s and 8MB per call" untrue: a hook can write gigabytes inside the deadline and all of it was kept for the length of the run. Capped at 256KB into the log. The filter inherited the whole environment, INPUT_GITHUB-TOKEN included, so a compiled hook could read the token the action runs with. That is a bigger risk than the one the README warned about, and it is now removed from the child environment along with the rest of the action's inputs. A relative path resolved inside the checkout and a bare name went through PATH, which is exactly the case the README told people to avoid. The path must now be absolute, checked once at setup so a bad one is reported rather than quietly producing nothing. Also fixed a test that passed for the wrong reason: a lone negative index is an inert map key, so it could not tell "answer voided" from "index ignored". It is paired with a valid index now. --- README.md | 9 ++-- action.yml | 2 +- internal/app/app.go | 7 ++- internal/git/diff.go | 80 ++++++++++++++++++++++----------- internal/git/hunkfilter_test.go | 43 +++++++++++++++++- pkg/hook/hook.go | 70 ++++++++++++++++++++++------- pkg/hook/hook_test.go | 44 ++++++++++++++++++ 7 files changed, 207 insertions(+), 48 deletions(-) diff --git a/README.md b/README.md index efd2de5..de275e9 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-83.4%25-brightgreen) +![Coverage](https://img.shields.io/badge/Coverage-83.5%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) @@ -425,8 +425,11 @@ 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 workflow naming the filter comes from the base branch, but a path inside `GITHUB_WORKSPACE` does not: the checkout is writable by the PR author, so a filter named there lets a contributor choose the code that judges their own changes. Install it in the runner image, fetch it by digest, or build it from a pinned ref. -* The program is executed directly rather than through a shell, once per distinct approved commit, bounded at 60s and 8MB of output per call. +* 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. +* Even so, `GITHUB_WORKSPACE` is writable by the PR author, so an absolute path pointing into the checkout is still the wrong thing to name. Install the program in the runner image, fetch it by digest, or build it from a pinned ref. +* 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 diff --git a/action.yml b/action.yml index f5800e5..a93626f 100644 --- a/action.yml +++ b/action.yml @@ -24,7 +24,7 @@ inputs: required: false default: false hunk-filter: - description: 'Path to an executable which reports already-reviewed post-approval hunks; see "Hunk Filters" in the README' + description: 'Absolute path to an executable which reports already-reviewed post-approval hunks; see "Hunk Filters" in the README' required: false default: '' diff --git a/internal/app/app.go b/internal/app/app.go index b9501a6..c6f0cf0 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "io" + "path/filepath" "slices" "strings" "time" @@ -175,7 +176,11 @@ func (a *App) Run() (*OutputData, error) { // Get the diff of the PR a.printDebug("Getting diff for %s...%s\n", diffContext.Base, diffContext.Head) var diffOpts []git.DiffOption - if a.config.HunkFilter != "" { + switch { + case a.config.HunkFilter == "": + case !filepath.IsAbs(a.config.HunkFilter): + a.printWarn("WARNING: hunk filter %q ignored: the path must be absolute\n", a.config.HunkFilter) + default: a.printDebug("Using hunk filter %s\n", a.config.HunkFilter) diffOpts = append(diffOpts, git.WithHunkFilter(a.hunkFilter(diffContext.Base, diffContext.Head))) } diff --git a/internal/git/diff.go b/internal/git/diff.go index de9c630..b074498 100644 --- a/internal/git/diff.go +++ b/internal/git/diff.go @@ -214,11 +214,13 @@ func changesSince(context changesSinceContext) ([]codeowners.DiffFile, error) { file := survivingHunks{name: diffToFilename(d)} for _, hunk := range d.Hunks { if !oldHunkHashes[hunkHash(hunk)] { - file.ranges = append(file.ranges, codeowners.HunkRange{ - Start: int(hunk.NewStartLine), - End: int(hunk.NewStartLine + hunk.NewLines - 1), + file.hunks = append(file.hunks, survivingHunk{ + rng: codeowners.HunkRange{ + Start: int(hunk.NewStartLine), + End: int(hunk.NewStartLine + hunk.NewLines - 1), + }, + body: string(hunk.Body), }) - file.bodies = append(file.bodies, string(hunk.Body)) } } survivors = append(survivors, file) @@ -232,32 +234,62 @@ func changesSince(context changesSinceContext) ([]codeowners.DiffFile, error) { 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(file.ranges) == 0 { + if len(file.hunks) == 0 { continue } diffFiles = append(diffFiles, codeowners.DiffFile{ FileName: file.name, - Hunks: file.ranges, + Hunks: file.ranges(), }) } return diffFiles, nil } -// Each range keeps the text it came from, so a filter can be asked about it. +type survivingHunk struct { + rng codeowners.HunkRange + body string +} + type survivingHunks struct { - name string - ranges []codeowners.HunkRange - bodies []string + name string + hunks []survivingHunk } -// 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 := make(map[string]bool, len(survivors)) +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.bodies) > 0 { - survivorNames[file.name] = true + 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 } @@ -276,12 +308,12 @@ func applyHunkFilter(context changesSinceContext, survivors []survivingHunks) [] files := make([]HunkText, 0, len(survivorNames)) for _, file := range survivors { - if len(file.bodies) == 0 { + if !survivorNames[file.name] { continue } files = append(files, HunkText{ Name: file.name, - HeadHunks: file.bodies, + HeadHunks: file.bodies(), ApprovalHunks: approvalBodies[file.name], }) } @@ -294,14 +326,14 @@ func applyHunkFilter(context changesSinceContext, survivors []survivingHunks) [] filtered := make([]survivingHunks, 0, len(survivors)) for _, file := range survivors { indexes, ok := reviewed[file.name] - if !ok || len(indexes) == 0 { + 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.ranges) { - // Answering about unsent hunks voids its answer for this file. + if index < 0 || index >= len(file.hunks) { + // An answer about an unsent hunk voids the answer for this file. drop = nil break } @@ -312,12 +344,10 @@ func applyHunkFilter(context changesSinceContext, survivors []survivingHunks) [] continue } kept := survivingHunks{name: file.name} - for i := range file.ranges { - if drop[i] { - continue + for i, hunk := range file.hunks { + if !drop[i] { + kept.hunks = append(kept.hunks, hunk) } - kept.ranges = append(kept.ranges, file.ranges[i]) - kept.bodies = append(kept.bodies, file.bodies[i]) } filtered = append(filtered, kept) } diff --git a/internal/git/hunkfilter_test.go b/internal/git/hunkfilter_test.go index 4e7d5d5..78f90a3 100644 --- a/internal/git/hunkfilter_test.go +++ b/internal/git/hunkfilter_test.go @@ -55,6 +55,47 @@ func hunkCounts(t *testing.T, filter HunkFilter) map[string]int { 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 { @@ -121,7 +162,7 @@ func TestHunkFilterFailuresChangeNothing(t *testing.T) { { name: "negative index", filter: func(string, []HunkText) (map[string][]int, error) { - return map[string][]int{"service.go": {-1}}, nil + return map[string][]int{"service.go": {0, -1}}, nil }, }, { diff --git a/pkg/hook/hook.go b/pkg/hook/hook.go index 11d7644..593f76e 100644 --- a/pkg/hook/hook.go +++ b/pkg/hook/hook.go @@ -7,7 +7,10 @@ import ( "encoding/json" "fmt" "io" + "os" "os/exec" + "path/filepath" + "strings" "time" ) @@ -20,22 +23,45 @@ const maxResponseBytes = 8 << 20 // block on a surviving grandchild for as long as it lived. const ioGrace = 2 * time.Second -// limitedBuffer keeps at most limit bytes, reporting every write as complete so the hook does not die on a short write. -type limitedBuffer struct { - buf bytes.Buffer +// 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 (b *limitedBuffer) Write(p []byte) (int, error) { - if room := b.limit - b.buf.Len(); len(p) > room { - b.overflow = true - if room > 0 { - b.buf.Write(p[:room]) +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 } - return len(p), nil + out = append(out, kv) } - return b.buf.Write(p) + return out } type Hunk struct { @@ -100,8 +126,19 @@ func (r Response) Indexes(req Request) (map[string][]int, error) { 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) @@ -109,11 +146,13 @@ func Run(ctx context.Context, path string, req Request, stderr io.Writer) (Respo return Response{}, fmt.Errorf("encoding hook request: %w", err) } - stdout := &limitedBuffer{limit: maxResponseBytes} + 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 = stderr + cmd.Stderr = &cappedWriter{to: stderr, limit: maxStderrBytes} + cmd.Env = withoutActionInputs(os.Environ()) cmd.WaitDelay = ioGrace if err := cmd.Run(); err != nil { @@ -126,7 +165,7 @@ func Run(ctx context.Context, path string, req Request, stderr io.Writer) (Respo return Response{}, fmt.Errorf("hook %s wrote more than the %d byte limit", path, maxResponseBytes) } - decoder := json.NewDecoder(&stdout.buf) + decoder := json.NewDecoder(&body) // An unknown field means the hook may believe it answered when it did not. decoder.DisallowUnknownFields() @@ -134,10 +173,7 @@ func Run(ctx context.Context, path string, req Request, stderr io.Writer) (Respo if err := decoder.Decode(&res); err != nil { return Response{}, fmt.Errorf("decoding hook response: %w", err) } - // More() reports false on a stray closing delimiter, so it cannot tell a clean - // end of stream from trailing junk. Only a second Decode returning EOF can. - var trailing json.RawMessage - if err := decoder.Decode(&trailing); err != io.EOF { + 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 index af7ec6b..21f934a 100644 --- a/pkg/hook/hook_test.go +++ b/pkg/hook/hook_test.go @@ -131,6 +131,50 @@ func TestRunFailsLoudly(t *testing.T) { } } +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 { From 81122638b796b2b7fb777af2db813fecad6e078b Mon Sep 17 00:00:00 2001 From: Ezekiel Lopez Date: Thu, 3 Sep 2026 22:30:47 -0700 Subject: [PATCH 4/4] move hunk filter into codeowners.toml and refuse GITHUB_WORKSPACE in checkout --- README.md | 21 ++++---- action.yml | 5 -- internal/app/app.go | 47 ++++++++++++++---- internal/app/hunk_filter_test.go | 83 +++++++++++++++++++++++++++++--- internal/config/config.go | 9 ++++ internal/config/config_test.go | 32 ++++++++++++ main.go | 22 ++++----- 7 files changed, 178 insertions(+), 41 deletions(-) diff --git a/README.md b/README.md index 7d409b8..75a8930 100644 --- a/README.md +++ b/README.md @@ -277,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: @@ -392,17 +396,15 @@ Using the `quiet` input on the action will change the behavior in a couple ways: 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. -Pass an executable to the action with the `hunk-filter` input: +Name an executable in the `[hooks]` table of `codeowners.toml`: -```yaml - - name: 'Codeowners Plus' - uses: multimediallc/codeowners-plus@v1 - with: - github-token: '${{ secrets.GITHUB_TOKEN }}' - pr: '${{ github.event.pull_request.number }}' - hunk-filter: '/opt/review-tools/already-seen' +```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 @@ -440,7 +442,8 @@ Notes: * 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. -* Even so, `GITHUB_WORKSPACE` is writable by the PR author, so an absolute path pointing into the checkout is still the wrong thing to name. Install the program in the runner image, fetch it by digest, or build it from a pinned ref. +* 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. diff --git a/action.yml b/action.yml index de17101..9b8b064 100644 --- a/action.yml +++ b/action.yml @@ -27,10 +27,6 @@ inputs: description: 'Disable PR comments and review requests' required: false default: false - hunk-filter: - description: 'Absolute path to an executable which reports already-reviewed post-approval hunks; see "Hunk Filters" in the README' - required: false - default: '' outputs: data: @@ -122,6 +118,5 @@ runs: INPUT_GITHUB-API-URL: ${{ inputs.github-api-url }} INPUT_VERBOSE: ${{ inputs.verbose }} INPUT_QUIET: ${{ inputs.quiet }} - INPUT_HUNK-FILTER: ${{ inputs.hunk-filter }} BIN: ${{ steps.resolve.outputs.bin }} run: '"${BIN}"' diff --git a/internal/app/app.go b/internal/app/app.go index c682926..23589a6 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -61,7 +61,7 @@ type Config struct { Repo string Verbose bool Quiet bool - HunkFilter string + Workspace string InfoBuffer io.Writer WarningBuffer io.Writer } @@ -110,7 +110,7 @@ func (a *App) printWarn(format string, args ...interface{}) { } // hunkFilter asks the hook which outstanding hunks are already reviewed; every failure is answered as "none". -func (a *App) hunkFilter(base, head string) git.HunkFilter { +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 { @@ -124,7 +124,7 @@ func (a *App) hunkFilter(base, head string) git.HunkFilter { ctx, cancel := context.WithTimeout(context.Background(), hunkFilterTimeout) defer cancel() - res, err := hook.Run(ctx, a.config.HunkFilter, req, a.config.WarningBuffer) + 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 @@ -138,6 +138,33 @@ func (a *App) hunkFilter(base, head string) git.HunkFilter { } } +// 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 { @@ -177,13 +204,13 @@ func (a *App) Run() (*OutputData, error) { // Get the diff of the PR a.printDebug("Getting diff for %s...%s\n", diffContext.Base, diffContext.Head) var diffOpts []git.DiffOption - switch { - case a.config.HunkFilter == "": - case !filepath.IsAbs(a.config.HunkFilter): - a.printWarn("WARNING: hunk filter %q ignored: the path must be absolute\n", a.config.HunkFilter) - default: - a.printDebug("Using hunk filter %s\n", a.config.HunkFilter) - diffOpts = append(diffOpts, git.WithHunkFilter(a.hunkFilter(diffContext.Base, diffContext.Head))) + 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 { diff --git a/internal/app/hunk_filter_test.go b/internal/app/hunk_filter_test.go index 76ee8c5..c3c9c51 100644 --- a/internal/app/hunk_filter_test.go +++ b/internal/app/hunk_filter_test.go @@ -36,9 +36,9 @@ func TestAppHunkFilterAppliesAValidAnswer(t *testing.T) { echo '{"reviewed":[{"name":"service.go","indexes":[1]}]}' `) warnings := &bytes.Buffer{} - a := &App{config: &Config{HunkFilter: path, WarningBuffer: warnings, InfoBuffer: &bytes.Buffer{}}} + a := &App{config: &Config{WarningBuffer: warnings, InfoBuffer: &bytes.Buffer{}}} - reviewed, err := a.hunkFilter("basesha", "headsha")("approvalsha", filterFiles()) + reviewed, err := a.hunkFilter(path, "basesha", "headsha")("approvalsha", filterFiles()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -69,12 +69,11 @@ func TestAppHunkFilterAnswersNoneOnFailure(t *testing.T) { t.Run(tc.name, func(t *testing.T) { warnings := &bytes.Buffer{} a := &App{config: &Config{ - HunkFilter: writeFilterHook(t, tc.body), WarningBuffer: warnings, InfoBuffer: &bytes.Buffer{}, }} - reviewed, err := a.hunkFilter("basesha", "headsha")("approvalsha", filterFiles()) + 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) } @@ -91,12 +90,11 @@ func TestAppHunkFilterAnswersNoneOnFailure(t *testing.T) { func TestAppHunkFilterMissingHookAnswersNone(t *testing.T) { warnings := &bytes.Buffer{} a := &App{config: &Config{ - HunkFilter: filepath.Join(t.TempDir(), "absent"), WarningBuffer: warnings, InfoBuffer: &bytes.Buffer{}, }} - reviewed, err := a.hunkFilter("basesha", "headsha")("approvalsha", filterFiles()) + 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) } @@ -107,3 +105,76 @@ func TestAppHunkFilterMissingHookAnswersNone(t *testing.T) { 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/main.go b/main.go index d07862f..636cd7d 100644 --- a/main.go +++ b/main.go @@ -16,14 +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 - HunkFilter *string + Token *string + ApiUrl *string + RepoDir *string + PR *int + Repo *string + Verbose *bool + Quiet *bool + Workspace *string } var ( @@ -39,8 +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"), - HunkFilter: flag.String("hunk-filter", getEnv("INPUT_HUNK-FILTER", ""), - "Path to an executable which reports already-reviewed hunks (see README)"), + // -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{}) @@ -154,7 +154,7 @@ func main() { Repo: *flags.Repo, Verbose: *flags.Verbose, Quiet: *flags.Quiet, - HunkFilter: *flags.HunkFilter, + Workspace: *flags.Workspace, InfoBuffer: InfoBuffer, WarningBuffer: WarningBuffer, }