codeowners: allow owner signoff through Comment reviews

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.0%25-brightgreen)
+![Coverage](https://img.shields.io/badge/Coverage-84.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)
 
@@ -113,6 +113,41 @@ It is recommended to also set up a rerun workflow on `pull_request_review` to re
 
 If you plan to have organization teams as code owners, you will need to use a PAT that has organization [read access for Members and Administration](https://docs.github.com/en/rest/authentication/permissions-required-for-fine-grained-personal-access-tokens) as the token. If you do not have organization teams as owners, [GITHUB_TOKEN](https://docs.github.com/en/actions/security-for-github-actions/security-guides/automatic-token-authentication#using-the-github_token-in-a-workflow) should be sufficient.
 
+### Owner-only signoff
+
+To satisfy ownership requirements without approving the pull request, submit a
+GitHub **Comment** review whose body includes this standalone line:
+
+```text
+codeowners-approved
+```
+
+The signer satisfies every required owner group their user or team membership
+can satisfy under the existing `.codeowners` rules. This does not create an
+`APPROVED` review, count toward `min_reviews` or `max_reviews`, or cause
+`enforcement.approval` to approve a PR whose ownership depends on the signoff.
+Ordinary approving reviews continue to satisfy ownership and review counts.
+PR-author handling remains controlled by `allow_self_approval`.
+
+Recognition is case-insensitive and allows up to three leading spaces and
+trailing spaces/tabs. Prose, inline backticks, blockquotes, list items, indented
+code, and backtick/tilde fenced code do not count. Leave a blank line after a
+quote, list, or HTML paragraph before writing the directive.
+
+Unrelated later comments preserve a signoff. Editing a review to remove or
+invalidate its directive withdraws that review's signoff; any other valid
+signoff reviews remain effective. A later **Request changes** or dismissed
+review supersedes earlier signoffs by that reviewer. A new Comment review can
+sign off again. Signoffs survive pushes with `disable_smart_dismissal = true`;
+otherwise, changes to the signer's owned files invalidate them using the same
+diff checks as ordinary approvals. Stale Comment reviews are ignored rather
+than dismissed through GitHub.
+
+Integrations must rerun Codeowners Plus on review submission, dismissal, and
+body edits adding or removing a valid directive. The parser compatibility
+fixtures are in `pkg/directives/testdata/codeowners_approved.json`; webhook
+receivers should validate against those same cases.
+
 ## Configuration
 
 ### .codeowners File Spec

internal/app/app.go

@@ -248,6 +248,12 @@ func (a *App) processApprovalsAndReviewers() (bool, string, []string, error) {
 		return false, message, nil, err
 	}
 
+	requiredBeforeSignoffs := len(a.codeowners.AllRequired())
+	if err := a.processOwnerSignoffs(); err != nil {
+		return false, message, nil, err
+	}
+	usedOwnerSignoff := len(a.codeowners.AllRequired()) < requiredBeforeSignoffs
+
 	// Request reviews from required owners
 	err = a.requestReviews()
 	if err != nil {
@@ -349,7 +355,7 @@ func (a *App) processApprovalsAndReviewers() (bool, string, []string, error) {
 	}
 
 	message = "Codeowners reviews satisfied"
-	if a.Conf.Enforcement.Approval && tokenOwnerApproval == nil {
+	if a.Conf.Enforcement.Approval && tokenOwnerApproval == nil && !usedOwnerSignoff {
 		// Approve the PR since all codeowner teams have approved
 		err = a.client.ApprovePR()
 		if err != nil {
@@ -515,6 +521,37 @@ func (a *App) processApprovals(ghApprovals []*gh.CurrentApproval) (int, error) {
 	return len(ghApprovals) - len(approvalsToDismiss), nil
 }
 
+func (a *App) processOwnerSignoffs() error {
+	signoffs, err := a.client.GetCurrentOwnerSignoffs()
+	if err != nil {
+		return fmt.Errorf("GetCurrentOwnerSignoffs Error: %v", err)
+	}
+	if len(signoffs) == 0 {
+		return nil
+	}
+	var approvers []codeowners.Slug
+	var stale []*gh.CurrentApproval
+	if a.Conf.DisableSmartDismissal {
+		for _, signoff := range signoffs {
+			approvers = append(approvers, signoff.Reviewers...)
+		}
+	} else {
+		fileReviewers := f.MapMap(a.codeowners.FileRequired(), func(reviewers codeowners.ReviewerGroups) []string {
+			return codeowners.NormalizedStrings(reviewers.Flatten())
+		})
+		approvers, stale = a.client.CheckApprovals(fileReviewers, signoffs, a.gitDiff)
+	}
+	for _, signoff := range signoffs {
+		if !slices.Contains(stale, signoff) {
+			a.printDebug("Owner signoff: %s (review %d, commit %s) satisfies %s\n",
+				signoff.GHLogin.Original(), signoff.ReviewID, signoff.CommitID, codeowners.OriginalStrings(signoff.Reviewers))
+		}
+	}
+	a.printDebug("Stale owner signoffs (ignored): %+v\n", stale)
+	a.codeowners.ApplyApprovals(approvers)
+	return nil
+}
+
 func (a *App) requestReviews() error {
 	if a.config.Quiet {
 		return nil

internal/app/signoffs_test.go

@@ -0,0 +1,153 @@
+package app
+
+import (
+	"errors"
+	"io"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/google/go-github/v63/github"
+	owners "github.com/multimediallc/codeowners-plus/internal/config"
+	"github.com/multimediallc/codeowners-plus/internal/git"
+	gh "github.com/multimediallc/codeowners-plus/internal/github"
+	"github.com/multimediallc/codeowners-plus/pkg/codeowners"
+)
+
+func (m *mockGitHubClient) GetCurrentOwnerSignoffs() ([]*gh.CurrentApproval, error) {
+	return nil, nil
+}
+
+type signoffClient struct {
+	mockGitHubClient
+	signoffs      []*gh.CurrentApproval
+	signoffsError error
+	approved      bool
+	dismissed     []*gh.CurrentApproval
+}
+
+func (m *signoffClient) GetCurrentOwnerSignoffs() ([]*gh.CurrentApproval, error) {
+	return m.signoffs, m.signoffsError
+}
+
+func (m *signoffClient) FindUserApproval(user string) (*gh.CurrentApproval, error) {
+	return nil, nil
+}
+
+func (m *signoffClient) ApprovePR() error {
+	m.approved = true
+	return nil
+}
+
+func (m *signoffClient) DismissStaleReviews(approvals []*gh.CurrentApproval) error {
+	m.dismissed = append(m.dismissed, approvals...)
+	return nil
+}
+
+func (m *signoffClient) CheckApprovals(fileReviewers map[string][]string, approvals []*gh.CurrentApproval, diff git.Diff) ([]codeowners.Slug, []*gh.CurrentApproval) {
+	return gh.NewClient("org", "repo", "").CheckApprovals(fileReviewers, approvals, diff)
+}
+
+func newSignoffApp(t *testing.T, client *signoffClient, files []string) *App {
+	t.Helper()
+	root := t.TempDir()
+	if err := os.WriteFile(filepath.Join(root, ".codeowners"), []byte("* @org/api @fallback\n&* @org/core\nother.go @other\n"), 0600); err != nil {
+		t.Fatal(err)
+	}
+	diff := mockGitDiff{changes: files}
+	co, err := codeowners.New(root, diff.AllChanges(), nil, io.Discard)
+	if err != nil {
+		t.Fatal(err)
+	}
+	return &App{
+		config: &Config{Quiet: true, InfoBuffer: io.Discard, WarningBuffer: io.Discard},
+		client: client, codeowners: co, gitDiff: diff,
+		Conf: &owners.Config{
+			DisableSmartDismissal: true,
+			Enforcement:           &owners.Enforcement{Approval: true, FailCheck: true},
+		},
+	}
+}
+
+func TestOwnerSignoffsSatisfyOwnershipOnly(t *testing.T) {
+	tt := []struct {
+		name              string
+		reviewers         []string
+		files             []string
+		ordinaryApprovals []*gh.CurrentApproval
+		minReviews        *int
+		maxReviews        *int
+		expectedSuccess   bool
+		expectedApproval  bool
+	}{
+		{name: "all authorized AND/OR groups", reviewers: []string{"@org/api", "@org/core"}, files: []string{"api.go"}, expectedSuccess: true},
+		{name: "outsider cannot sign off", files: []string{"api.go"}},
+		{name: "unrelated owner remains required", reviewers: []string{"@org/api", "@org/core"}, files: []string{"api.go", "other.go"}},
+		{name: "signoff does not meet minimum", reviewers: []string{"@org/api", "@org/core"}, files: []string{"api.go"}, minReviews: github.Int(1)},
+		{name: "signoff does not reach maximum", reviewers: []string{"@org/api"}, files: []string{"api.go"}, maxReviews: github.Int(1)},
+		{name: "ordinary approval plus signoff", reviewers: []string{"@org/api", "@org/core"}, files: []string{"api.go", "other.go"}, ordinaryApprovals: []*gh.CurrentApproval{{Reviewers: codeowners.NewSlugs([]string{"@other"})}}, minReviews: github.Int(1), expectedSuccess: true},
+		{name: "ordinary approvals retain enforcement", files: []string{"api.go"}, ordinaryApprovals: []*gh.CurrentApproval{{Reviewers: codeowners.NewSlugs([]string{"@org/api", "@org/core"})}}, expectedSuccess: true, expectedApproval: true},
+		{name: "redundant signoff retains enforcement", reviewers: []string{"@org/api", "@org/core"}, files: []string{"api.go"}, ordinaryApprovals: []*gh.CurrentApproval{{Reviewers: codeowners.NewSlugs([]string{"@org/api", "@org/core"})}}, expectedSuccess: true, expectedApproval: true},
+	}
+	for _, tc := range tt {
+		t.Run(tc.name, func(t *testing.T) {
+			client := &signoffClient{mockGitHubClient: mockGitHubClient{currentApprovals: tc.ordinaryApprovals}}
+			if tc.reviewers != nil {
+				client.signoffs = []*gh.CurrentApproval{{GHLogin: codeowners.NewSlug("signer"), ReviewID: 10, Reviewers: codeowners.NewSlugs(tc.reviewers), CommitID: "old-head"}}
+			}
+			app := newSignoffApp(t, client, tc.files)
+			app.Conf.MinReviews = tc.minReviews
+			app.Conf.MaxReviews = tc.maxReviews
+			success, message, _, err := app.processApprovalsAndReviewers()
+			if err != nil || success != tc.expectedSuccess {
+				t.Fatalf("success = %v, expected %v; %s; error = %v", success, tc.expectedSuccess, message, err)
+			}
+			if client.approved != tc.expectedApproval {
+				t.Errorf("GitHub approval created = %v, expected %v", client.approved, tc.expectedApproval)
+			}
+			if len(client.dismissed) > 0 {
+				t.Errorf("unexpected dismissals: %v", client.dismissed)
+			}
+		})
+	}
+}
+
+func TestOwnerSignoffLifetime(t *testing.T) {
+	tt := []struct {
+		name            string
+		disableSmart    bool
+		changes         []string
+		diffError       error
+		expectedSuccess bool
+	}{
+		{name: "persists across pushes when dismissal disabled", disableSmart: true, changes: []string{"api.go"}, expectedSuccess: true},
+		{name: "owned changes invalidate", changes: []string{"api.go"}},
+		{name: "unrelated changes preserve", changes: []string{"unrelated.go"}, expectedSuccess: true},
+		{name: "unchanged commit preserves", expectedSuccess: true},
+		{name: "unreadable commit invalidates", diffError: errors.New("missing commit")},
+	}
+	for _, tc := range tt {
+		t.Run(tc.name, func(t *testing.T) {
+			client := &signoffClient{signoffs: []*gh.CurrentApproval{{Reviewers: codeowners.NewSlugs([]string{"@org/api", "@org/core"}), CommitID: "reviewed"}}}
+			app := newSignoffApp(t, client, []string{"api.go"})
+			app.Conf.DisableSmartDismissal = tc.disableSmart
+			app.gitDiff = mockGitDiff{changes: tc.changes, changesSinceError: tc.diffError}
+			success, message, _, err := app.processApprovalsAndReviewers()
+			if err != nil || success != tc.expectedSuccess {
+				t.Fatalf("success = %v, expected %v; %s; error = %v", success, tc.expectedSuccess, message, err)
+			}
+			if client.approved || len(client.dismissed) > 0 {
+				t.Errorf("signoff caused GitHub review mutation: approved=%v dismissed=%v", client.approved, client.dismissed)
+			}
+		})
+	}
+}
+
+func TestOwnerSignoffReadError(t *testing.T) {
+	client := &signoffClient{signoffsError: errors.New("review read failed")}
+	app := newSignoffApp(t, client, []string{"api.go"})
+	success, _, _, err := app.processApprovalsAndReviewers()
+	if err == nil || success {
+		t.Fatalf("expected failure, got success=%v error=%v", success, err)
+	}
+}

internal/github/gh.go

@@ -38,6 +38,7 @@ type Client interface {
 	AllApprovals() ([]*CurrentApproval, error)
 	FindUserApproval(ghUser string) (*CurrentApproval, error)
 	GetCurrentReviewerApprovals() ([]*CurrentApproval, error)
+	GetCurrentOwnerSignoffs() ([]*CurrentApproval, error)
 	GetAlreadyReviewed() ([]codeowners.Slug, error)
 	GetCurrentlyRequested() ([]codeowners.Slug, error)
 	DismissStaleReviews(staleApprovals []*CurrentApproval) error

internal/github/signoffs.go

@@ -0,0 +1,40 @@
+package gh
+
+import (
+	"strings"
+
+	"github.com/google/go-github/v63/github"
+	"github.com/multimediallc/codeowners-plus/pkg/directives"
+)
+
+func (gh *GHClient) GetCurrentOwnerSignoffs() ([]*CurrentApproval, error) {
+	if gh.pr == nil {
+		return nil, &NoPRError{}
+	}
+	if gh.userReviewerMap == nil {
+		return nil, &UserReviewerMapNotInitError{}
+	}
+	if gh.reviews == nil {
+		if err := gh.InitReviews(); err != nil {
+			return nil, err
+		}
+	}
+	seen := make(map[string]bool)
+	signoffs := make([]*github.PullRequestReview, 0)
+	for _, review := range gh.reviews {
+		user := strings.ToLower(review.GetUser().GetLogin())
+		if seen[user] {
+			continue
+		}
+		switch review.GetState() {
+		case "CHANGES_REQUESTED", "DISMISSED":
+			seen[user] = true
+		case "COMMENTED":
+			if directives.HasCodeownersApproval(review.GetBody()) {
+				seen[user] = true
+				signoffs = append(signoffs, review)
+			}
+		}
+	}
+	return currentReviewerApprovalsFromReviews(signoffs, gh.userReviewerMap), nil
+}