From cb384e0a0547b9d106c1ae059d38a4ce2005554b Mon Sep 17 00:00:00 2001 From: Christopher Tineo Date: Wed, 13 May 2026 09:35:35 +0100 Subject: [PATCH 1/2] restrict newcomer self-assignment in assign plugin Block non-member self-assignment on issues that are not marked good-first-issue, provide clearer retry and guidance comments, and refactor the assign flow into smaller helpers to keep the happy path flat. --- pkg/plugins/assign/assign.go | 157 +++++++++++++++++++++++++++--- pkg/plugins/assign/assign_test.go | 112 ++++++++++++++++++++- 2 files changed, 251 insertions(+), 18 deletions(-) diff --git a/pkg/plugins/assign/assign.go b/pkg/plugins/assign/assign.go index c44d71b062..6470f3fea2 100644 --- a/pkg/plugins/assign/assign.go +++ b/pkg/plugins/assign/assign.go @@ -35,8 +35,11 @@ var ( assignRe = regexp.MustCompile(`(?mi)^/(un)?assign(( @?[-\w]+?)*)\s*$`) // CCRegexp parses and validates /cc commands, also used by blunderbuss CCRegexp = regexp.MustCompile(`(?mi)^/(un)?cc(( +@?[-/\w]+?)*)\s*$`) + contributorGuidePaths = []string{"CONTRIBUTING.md", "docs/CONTRIBUTING.md"} ) +const defaultHelpGuidelinesURL = "https://git.k8s.io/community/contributors/guide/help-wanted.md" + func init() { plugins.RegisterGenericCommentHandler(pluginName, handleGenericComment, helpProvider) } @@ -71,13 +74,20 @@ type githubClient interface { UnrequestReview(org, repo string, number int, logins []string) error CreateComment(owner, repo string, number int, comment string) error + IsMember(org, user string) (bool, error) + GetIssueLabels(org, repo string, number int) ([]github.Label, error) + GetFile(org, repo, filepath, commit string) ([]byte, error) } func handleGenericComment(pc plugins.Agent, e github.GenericCommentEvent) error { if e.Action != github.GenericCommentActionCreated { return nil } - err := handle(newAssignHandler(e, pc.GitHubClient, pc.Logger)) + helpGuidelinesURL := "" + if pc.PluginConfig != nil { + helpGuidelinesURL = pc.PluginConfig.Help.HelpGuidelinesURL + } + err := handle(newAssignHandler(e, pc.GitHubClient, pc.Logger, helpGuidelinesURL)) if e.IsPR { err = combineErrors(err, handle(newReviewHandler(e, pc.GitHubClient, pc.Logger))) } @@ -144,26 +154,140 @@ func handle(h *handler) error { return err } } - if len(toAdd) > 0 { - h.log.Printf("Adding %s to %s/%s#%d: %v", h.userType, org, repo, e.Number, toAdd) - if err := h.add(org, repo, e.Number, toAdd); err != nil { - if mu, ok := err.(github.MissingUsers); ok { - msg := h.addFailureResponse(mu) - if len(msg) == 0 { - return nil - } - h.log.Printf("Failed to add %s to %s/%s#%d: %s", h.userType, org, repo, e.Number, mu.Error()) - if err := h.gc.CreateComment(org, repo, e.Number, plugins.FormatResponseRaw(e.Body, e.HTMLURL, e.User.Login, msg)); err != nil { - return fmt.Errorf("comment err: %w", err) - } + if len(toAdd) == 0 { + return nil + } + + h.log.Printf("Adding %s to %s/%s#%d: %v", h.userType, org, repo, e.Number, toAdd) + if err := h.maybeRestrictSelfAssign(org, repo, users[e.User.Login], &toAdd); err != nil { + return err + } + if len(toAdd) == 0 { + return nil + } + return h.addUsersOrComment(org, repo, toAdd) +} + +func (h *handler) maybeRestrictSelfAssign(org, repo string, selfAssign bool, toAdd *[]string) error { + if !h.shouldRestrictSelfAssign(selfAssign) { + return nil + } + + allowed, retryMsg, err := h.canSelfAssign(org, repo) + if err != nil { + return err + } + if retryMsg != "" { + return h.replyAndRemoveSelf(org, repo, toAdd, retryMsg) + } + if allowed { + return nil + } + + h.replyWithNewContributorGuidance(org, repo, toAdd) + return nil +} + +func (h *handler) shouldRestrictSelfAssign(selfAssign bool) bool { + return h.userType == "assignee(s)" && selfAssign +} + +func (h *handler) canSelfAssign(org, repo string) (allowed bool, retryMsg string, err error) { + labels, err := h.gc.GetIssueLabels(org, repo, h.event.Number) + if err != nil { + h.log.WithError(err).Warn("Failed to get issue labels") + return false, "I couldn't check whether this issue is marked `good-first-issue` right now. Please try `/assign` again later.", nil + } + if hasGoodFirstIssue(labels) { + return true, "", nil + } + + isMember, err := h.gc.IsMember(org, h.event.User.Login) + if err != nil { + h.log.WithError(err).Warn("Failed to check membership") + return false, "I couldn't verify your organization membership right now. Please try `/assign` again later.", nil + } + + return isMember, "", nil +} + +func (h *handler) addUsersOrComment(org, repo string, toAdd []string) error { + if err := h.add(org, repo, h.event.Number, toAdd); err != nil { + if mu, ok := err.(github.MissingUsers); ok { + msg := h.addFailureResponse(mu) + if len(msg) == 0 { return nil } - return err + h.log.Printf("Failed to add %s to %s/%s#%d: %s", h.userType, org, repo, h.event.Number, mu.Error()) + if err := h.gc.CreateComment(org, repo, h.event.Number, plugins.FormatResponseRaw(h.event.Body, h.event.HTMLURL, h.event.User.Login, msg)); err != nil { + return fmt.Errorf("comment err: %w", err) + } + return nil } + return err + } + + return nil +} + +func (h *handler) replyAndRemoveSelf(org, repo string, toAdd *[]string, msg string) error { + if err := h.gc.CreateComment(org, repo, h.event.Number, plugins.FormatResponseRaw(h.event.Body, h.event.HTMLURL, h.event.User.Login, msg)); err != nil { + return fmt.Errorf("comment err: %w", err) } + *toAdd = removeLogin(*toAdd, h.event.User.Login) return nil } +func (h *handler) replyWithNewContributorGuidance(org, repo string, toAdd *[]string) { + msg := h.newContributorMessage(org, repo) + if err := h.gc.CreateComment(org, repo, h.event.Number, plugins.FormatResponseRaw(h.event.Body, h.event.HTMLURL, h.event.User.Login, msg)); err != nil { + h.log.WithError(err).Warn("Failed to create educational comment") + } + *toAdd = removeLogin(*toAdd, h.event.User.Login) +} + +func (h *handler) newContributorMessage(org, repo string) string { + return fmt.Sprintf("Thank you for your interest in contributing! It looks like you're new, and this issue hasn't been vetted for beginners yet. Please check out the [Good First Issues List](https://github.com/%s/%s/labels/good-first-issue)%s to get started.", org, repo, h.contributorGuideLink(org, repo)) +} + +func (h *handler) contributorGuideLink(org, repo string) string { + defaultBranch := h.event.Repo.DefaultBranch + if defaultBranch == "" { + defaultBranch = "main" + } + + for _, path := range contributorGuidePaths { + if _, err := h.gc.GetFile(org, repo, path, defaultBranch); err == nil { + return fmt.Sprintf(" and our [contributor guide](https://github.com/%s/%s/blob/%s/%s)", org, repo, defaultBranch, path) + } + } + + if h.helpGuidelinesURL != "" && h.helpGuidelinesURL != defaultHelpGuidelinesURL { + return fmt.Sprintf(" and our [contributor guide](%s)", h.helpGuidelinesURL) + } + + return "" +} + +func hasGoodFirstIssue(labels []github.Label) bool { + for _, label := range labels { + if strings.ToLower(label.Name) == "good-first-issue" { + return true + } + } + return false +} + +func removeLogin(logins []string, login string) []string { + filtered := logins[:0] + for _, current := range logins { + if current != login { + filtered = append(filtered, current) + } + } + return filtered +} + // handler is a struct that contains data about a github event and provides functions to help handle it. type handler struct { // addFailureResponse generates the body of a response comment in the event that the add function fails. @@ -185,9 +309,11 @@ type handler struct { log *logrus.Entry // userType is a string that represents the type of users affected by this handler. (e.g. 'assignees') userType string + // helpGuidelinesURL is used as a fallback when the repo does not have a local contributor guide. + helpGuidelinesURL string } -func newAssignHandler(e github.GenericCommentEvent, gc githubClient, log *logrus.Entry) *handler { +func newAssignHandler(e github.GenericCommentEvent, gc githubClient, log *logrus.Entry, helpGuidelinesURL string) *handler { org := e.Repo.Owner.Login addFailureResponse := func(mu github.MissingUsers) string { return fmt.Sprintf("GitHub didn't allow me to assign the following users: %s.\n\nNote that only [%s members](https://%s/orgs/%s/people) with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.\nFor more information please see [the contributor guide](https://git.k8s.io/community/contributors/guide/first-contribution.md#issue-assignment-in-github)", strings.Join(mu.Users, ", "), org, github.DefaultHost, org) @@ -202,6 +328,7 @@ func newAssignHandler(e github.GenericCommentEvent, gc githubClient, log *logrus gc: gc, log: log, userType: "assignee(s)", + helpGuidelinesURL: helpGuidelinesURL, } } diff --git a/pkg/plugins/assign/assign_test.go b/pkg/plugins/assign/assign_test.go index 36a4aa09a9..2f7e1be248 100644 --- a/pkg/plugins/assign/assign_test.go +++ b/pkg/plugins/assign/assign_test.go @@ -17,7 +17,9 @@ limitations under the License. package assign import ( + "fmt" "sort" + "strings" "testing" "github.com/sirupsen/logrus" @@ -34,6 +36,39 @@ type fakeClient struct { contributors map[string]bool commented bool + + isMember map[string]bool + labels []github.Label + + getIssueLabelsErr error + isMemberErr error + + lastComment string + isMemberCalled bool +} + +func (c *fakeClient) IsMember(org, user string) (bool, error) { + c.isMemberCalled = true + if c.isMemberErr != nil { + return false, c.isMemberErr + } + return c.isMember[user], nil +} + +func (c *fakeClient) GetIssueLabels(org, repo string, number int) ([]github.Label, error) { + if c.getIssueLabelsErr != nil { + return nil, c.getIssueLabelsErr + } + return c.labels, nil +} + +func (c *fakeClient) GetFile(org, repo, filepath, commit string) ([]byte, error) { + for _, path := range contributorGuidePaths { + if filepath == path { + return []byte("content"), nil + } + } + return nil, fmt.Errorf("file not found") } func (c *fakeClient) UnassignIssue(owner, repo string, number int, assignees []string) error { @@ -92,6 +127,7 @@ func (c *fakeClient) UnrequestReview(org, repo string, number int, logins []stri func (c *fakeClient) CreateComment(owner, repo string, number int, comment string) error { c.commented = comment != "" + c.lastComment = comment return nil } @@ -106,6 +142,7 @@ func newFakeClient(contribs []string) *fakeClient { for _, user := range contribs { c.contributors[user] = true } + c.isMember = make(map[string]bool) return c } @@ -165,6 +202,9 @@ func TestAssignAndReview(t *testing.T) { requested []string unrequested []string commented bool + expectedMsg string + labelErr error + memberErr error }{ { name: "unrelated comment", @@ -386,16 +426,69 @@ func TestAssignAndReview(t *testing.T) { commenter: "rando", unrequested: []string{"kubernetes/sig-testing-misc"}, }, + { + name: "non-member assigns themselves to non-good-first-issue, sends educational message", + body: "/assign", + commenter: "newbie", + commented: true, + expectedMsg: "Thank you for your interest in contributing! It looks like you're new, and this issue hasn't been vetted for beginners yet. Please check out the [Good First Issues List](https://github.com/org/repo/labels/good-first-issue) and our [contributor guide](https://github.com/org/repo/blob/main/CONTRIBUTING.md) to get started.", + }, + { + name: "non-member assigns themselves to good-first-issue, no educational message", + body: "/assign", + commenter: "newbie", + assigned: []string{"newbie"}, + }, + { + name: "org member assigns themselves to non-good-first-issue, no educational message", + body: "/assign", + commenter: "member", + assigned: []string{"member"}, + }, + { + name: "non-member assigns another user, no educational message", + body: "/assign @member", + commenter: "newbie", + assigned: []string{"member"}, + }, + { + name: "label lookup failure asks commenter to retry", + body: "/assign", + commenter: "newbie", + commented: true, + expectedMsg: "I couldn't check whether this issue is marked `good-first-issue` right now. Please try `/assign` again later.", + labelErr: fmt.Errorf("boom"), + }, + { + name: "membership lookup failure asks commenter to retry", + body: "/assign", + commenter: "newbie", + commented: true, + expectedMsg: "I couldn't verify your organization membership right now. Please try `/assign` again later.", + memberErr: fmt.Errorf("boom"), + }, } for _, tc := range testcases { - fc := newFakeClient([]string{"hello-world", "allow_underscore", "cjwagner", "merlin", "kubernetes/sig-testing-misc"}) + fc := newFakeClient([]string{"hello-world", "allow_underscore", "cjwagner", "merlin", "kubernetes/sig-testing-misc", "newbie", "member", "rando", "o", "san", "innocent"}) + // Default to member for existing tests + for _, u := range []string{"hello-world", "allow_underscore", "cjwagner", "merlin", "member", "rando", "o", "san", "innocent", "evil"} { + fc.isMember[u] = true + } + if tc.commenter == "member" { + fc.isMember["member"] = true + } + if tc.name == "non-member assigns themselves to good-first-issue, no educational message" { + fc.labels = []github.Label{{Name: "good-first-issue"}} + } + fc.getIssueLabelsErr = tc.labelErr + fc.isMemberErr = tc.memberErr e := github.GenericCommentEvent{ Body: tc.body, User: github.User{Login: tc.commenter}, - Repo: github.Repo{Name: "repo", Owner: github.User{Login: "org"}}, + Repo: github.Repo{Name: "repo", Owner: github.User{Login: "org"}, DefaultBranch: "main"}, Number: 5, } - if err := handle(newAssignHandler(e, fc, logrus.WithField("plugin", pluginName))); err != nil { + if err := handle(newAssignHandler(e, fc, logrus.WithField("plugin", pluginName), "")); err != nil { t.Errorf("For case %s, didn't expect error from handle: %v", tc.name, err) continue } @@ -407,6 +500,9 @@ func TestAssignAndReview(t *testing.T) { if tc.commented != fc.commented { t.Errorf("For case %s, expect commented: %v, got commented %v", tc.name, tc.commented, fc.commented) } + if tc.expectedMsg != "" && !strings.Contains(fc.lastComment, tc.expectedMsg) { + t.Errorf("For case %s, expected comment to contain %q, but got %q", tc.name, tc.expectedMsg, fc.lastComment) + } if len(fc.assigned) != len(tc.assigned) { t.Errorf("For case %s, assigned actual %v != expected %s", tc.name, fc.assigned, tc.assigned) @@ -449,5 +545,15 @@ func TestAssignAndReview(t *testing.T) { } } } + if tc.name == "non-member assigns themselves to good-first-issue, no educational message" { + if fc.isMemberCalled { + t.Errorf("For case %s, expected IsMember NOT to be called, but it was", tc.name) + } + } + if tc.name == "non-member assigns themselves to non-good-first-issue, sends educational message" { + if !fc.isMemberCalled { + t.Errorf("For case %s, expected IsMember to be called, but it wasn't", tc.name) + } + } } } From 773c479cad28d019abfabee63bdba190d2544280 Mon Sep 17 00:00:00 2001 From: Christopher Tineo Date: Wed, 13 May 2026 09:39:05 +0100 Subject: [PATCH 2/2] rename assign self-assignment helper Rename the self-assignment enforcement helper to better reflect that it applies the evaluated policy rather than conditionally doing work. --- pkg/plugins/assign/assign.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/plugins/assign/assign.go b/pkg/plugins/assign/assign.go index 6470f3fea2..99fc1d0036 100644 --- a/pkg/plugins/assign/assign.go +++ b/pkg/plugins/assign/assign.go @@ -159,7 +159,7 @@ func handle(h *handler) error { } h.log.Printf("Adding %s to %s/%s#%d: %v", h.userType, org, repo, e.Number, toAdd) - if err := h.maybeRestrictSelfAssign(org, repo, users[e.User.Login], &toAdd); err != nil { + if err := h.enforceSelfAssignRestriction(org, repo, users[e.User.Login], &toAdd); err != nil { return err } if len(toAdd) == 0 { @@ -168,7 +168,7 @@ func handle(h *handler) error { return h.addUsersOrComment(org, repo, toAdd) } -func (h *handler) maybeRestrictSelfAssign(org, repo string, selfAssign bool, toAdd *[]string) error { +func (h *handler) enforceSelfAssignRestriction(org, repo string, selfAssign bool, toAdd *[]string) error { if !h.shouldRestrictSelfAssign(selfAssign) { return nil }