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

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 156 additions & 0 deletions pkg/github/prcreation/githubapp.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
package prcreation

import (
"crypto/rsa"
"flag"
"fmt"
"os"
"strings"

"github.com/dgrijalva/jwt-go/v4"
"github.com/sirupsen/logrus"

"sigs.k8s.io/prow/pkg/config/secret"
"sigs.k8s.io/prow/pkg/github"
)

// GitHubAppOptions holds configuration for a secondary GitHub App client
// used exclusively for PR creation. Flag names are prefixed with "pr-" to
// avoid collision with prow's built-in --github-app-id / --github-app-private-key-path.
type GitHubAppOptions struct {
AppID string
PrivateKeyPath string

// client is lazily constructed via Finalize.
client github.Client
}

// AddFlags registers GitHub App flags on the given flag set.
func (o *GitHubAppOptions) AddFlags(fs *flag.FlagSet) {
fs.StringVar(&o.AppID, "pr-app-id", "", "GitHub App ID for PR creation (requires --pr-app-private-key-path)")
fs.StringVar(&o.PrivateKeyPath, "pr-app-private-key-path", "", "Path to the GitHub App private key PEM for PR creation (requires --pr-app-id)")
}

// Enabled returns true when both App fields are configured.
func (o *GitHubAppOptions) Enabled() bool {
return o.AppID != "" && o.PrivateKeyPath != ""
}

// Validate checks both-or-neither consistency and that the key file exists.
func (o *GitHubAppOptions) Validate() error {
if o.AppID == "" && o.PrivateKeyPath == "" {
return nil
}
if o.AppID == "" || o.PrivateKeyPath == "" {
return fmt.Errorf("--pr-app-id and --pr-app-private-key-path must both be set or both be empty")
}
if _, err := os.Stat(o.PrivateKeyPath); err != nil {
return fmt.Errorf("cannot access private key file %s: %w", o.PrivateKeyPath, err)
}
return nil
}

// Finalize constructs the App-authenticated GitHub client. Must be called
// after Validate and only when Enabled returns true.
func (o *GitHubAppOptions) Finalize() error {
if !o.Enabled() {
return nil
}

keyGenerator, err := secret.AddWithParser(
o.PrivateKeyPath,
func(raw []byte) (*rsa.PrivateKey, error) {
key, err := jwt.ParseRSAPrivateKeyFromPEM(raw)
if err != nil {
return nil, fmt.Errorf("failed to parse RSA private key from PEM: %w", err)
}
return key, nil
},
)
if err != nil {
return fmt.Errorf("failed to load GitHub App private key: %w", err)
}

opts := github.ClientOptions{
Censor: secret.Censor,
AppID: o.AppID,
AppPrivateKey: keyGenerator,
Bases: []string{github.DefaultAPIEndpoint},
}

_, _, client, err := github.NewClientFromOptions(logrus.Fields{
"client": "pr-github-app",
}, opts)
if err != nil {
return fmt.Errorf("failed to construct GitHub App client: %w", err)
}

o.client = client
return nil
}

// Client returns the App-authenticated GitHub client.
func (o *GitHubAppOptions) Client() github.Client {
return o.client
}

// UpsertPR creates or updates a pull request using the App client.
// It sets canModify=false to avoid fork_collab errors on cross-fork PRs.
func (o *GitHubAppOptions) UpsertPR(org, repo, base, head, title, body string, prLabels []string) error {
l := logrus.WithFields(logrus.Fields{"org": org, "repo": repo, "head": head})

prNumber, err := o.client.CreatePullRequest(org, repo, title, body, head, base, false)
if err != nil {
// GitHub returns 422 with "already exists" when a PR for this head already exists.
if !strings.Contains(err.Error(), "already exists") {
return fmt.Errorf("failed to create PR: %w", err)
}
l.Info("PR already exists, updating existing PR")
prNumber, err = o.findExistingPR(org, repo, head)
if err != nil {
return fmt.Errorf("failed to find existing PR: %w", err)
}
if err := o.client.UpdatePullRequest(org, repo, prNumber, &title, &body, nil, nil, nil); err != nil {
return fmt.Errorf("failed to update PR #%d: %w", prNumber, err)
}
}

l.WithField("number", prNumber).Info("PR created/updated via GitHub App")

if len(prLabels) > 0 {
if err := o.client.AddLabels(org, repo, prNumber, prLabels...); err != nil {
return fmt.Errorf("failed to add labels to PR #%d: %w", prNumber, err)
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

return nil
}

// findExistingPR lists open PRs and finds one matching the given head ref.
// head is typically in "owner:branch" format; we match against both the raw
// branch ref and the fully qualified "repo-owner:ref" form.
func (o *GitHubAppOptions) findExistingPR(org, repo, head string) (int, error) {
prs, err := o.client.GetPullRequests(org, repo)
if err != nil {
return 0, fmt.Errorf("failed to list PRs: %w", err)
}
// Derive the branch name from head, which may be "owner:branch" or just "branch".
headBranch := head
if i := strings.Index(head, ":"); i != -1 && i+1 < len(head) {
headBranch = head[i+1:]
}
for _, pr := range prs {
if pr.Head.Ref == headBranch {
return pr.Number, nil
}
// Guard against nil Head.Repo/Owner, which can occur for PRs from deleted forks.
if pr.Head.Repo.Owner.Login == "" {
continue
}
qualifiedRef := pr.Head.Repo.Owner.Login + ":" + pr.Head.Ref
if qualifiedRef == head {
return pr.Number, nil
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Match owner-qualified head before branch-only to avoid updating the wrong PR.

When head is owner:branch, the current branch-first match can pick another fork’s PR with the same branch name and update the wrong PR.

🔧 Suggested fix
 func (o *GitHubAppOptions) findExistingPR(org, repo, head string) (int, error) {
 	prs, err := o.client.GetPullRequests(org, repo)
 	if err != nil {
 		return 0, fmt.Errorf("failed to list PRs: %w", err)
 	}
 	// Derive the branch name from head, which may be "owner:branch" or just "branch".
+	headOwner := ""
 	headBranch := head
 	if i := strings.Index(head, ":"); i != -1 && i+1 < len(head) {
+		headOwner = head[:i]
 		headBranch = head[i+1:]
 	}
 	for _, pr := range prs {
-		if pr.Head.Ref == headBranch {
-			return pr.Number, nil
-		}
 		// Guard against nil Head.Repo/Owner, which can occur for PRs from deleted forks.
 		if pr.Head.Repo.Owner.Login == "" {
+			if headOwner == "" && pr.Head.Ref == headBranch {
+				return pr.Number, nil
+			}
 			continue
 		}
 		qualifiedRef := pr.Head.Repo.Owner.Login + ":" + pr.Head.Ref
-		if qualifiedRef == head {
+		if headOwner != "" && qualifiedRef == head {
 			return pr.Number, nil
 		}
+		if headOwner == "" && pr.Head.Ref == headBranch {
+			return pr.Number, nil
+		}
 	}
 	return 0, fmt.Errorf("no open PR found matching head %q", head)
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for _, pr := range prs {
if pr.Head.Ref == headBranch {
return pr.Number, nil
}
// Guard against nil Head.Repo/Owner, which can occur for PRs from deleted forks.
if pr.Head.Repo.Owner.Login == "" {
continue
}
qualifiedRef := pr.Head.Repo.Owner.Login + ":" + pr.Head.Ref
if qualifiedRef == head {
return pr.Number, nil
}
func (o *GitHubAppOptions) findExistingPR(org, repo, head string) (int, error) {
prs, err := o.client.GetPullRequests(org, repo)
if err != nil {
return 0, fmt.Errorf("failed to list PRs: %w", err)
}
// Derive the branch name from head, which may be "owner:branch" or just "branch".
headOwner := ""
headBranch := head
if i := strings.Index(head, ":"); i != -1 && i+1 < len(head) {
headOwner = head[:i]
headBranch = head[i+1:]
}
for _, pr := range prs {
// Guard against nil Head.Repo/Owner, which can occur for PRs from deleted forks.
if pr.Head.Repo.Owner.Login == "" {
if headOwner == "" && pr.Head.Ref == headBranch {
return pr.Number, nil
}
continue
}
qualifiedRef := pr.Head.Repo.Owner.Login + ":" + pr.Head.Ref
if headOwner != "" && qualifiedRef == head {
return pr.Number, nil
}
if headOwner == "" && pr.Head.Ref == headBranch {
return pr.Number, nil
}
}
return 0, fmt.Errorf("no open PR found matching head %q", head)
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/github/prcreation/githubapp.go` around lines 142 - 153, The loop that
finds a matching PR checks branch-only (pr.Head.Ref == headBranch) before the
owner-qualified match and can pick the wrong PR; in the block iterating prs,
first guard against nil pr.Head/Repo/Repo.Owner, then compute qualifiedRef
(pr.Head.Repo.Owner.Login + ":" + pr.Head.Ref) and check if qualifiedRef == head
and return pr.Number, nil; only if that fails, fall back to the branch-only
check (pr.Head.Ref == headBranch). Update the logic around the existing
variables head, headBranch, pr.Head.Ref and pr.Head.Repo.Owner.Login to perform
the owner-qualified match first to avoid updating the wrong PR.

}
return 0, fmt.Errorf("no open PR found matching head %q", head)
}
Comment on lines +1 to +137
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new GitHub App authentication functionality introduced in this file lacks test coverage. Consider adding unit tests to verify:

  1. The validation logic (both fields required together)
  2. The PR creation and update flow
  3. The existing PR finding logic with various head ref formats
  4. Error handling for API failures
  5. Edge cases like deleted forks (empty owner login)

Copilot uses AI. Check for mistakes.
46 changes: 39 additions & 7 deletions pkg/github/prcreation/prcreation.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,21 @@ import (

type PRCreationOptions struct {
SelfApprove bool
GitHubApp GitHubAppOptions
flagutil.GitHubOptions
GithubClient github.Client
}

func (o *PRCreationOptions) AddFlags(fs *flag.FlagSet) {
fs.BoolVar(&o.SelfApprove, "self-approve", false, "If the created PR should be self-approved by adding the lgtm+approved labels")
o.GitHubOptions.AddFlags(fs)
o.GitHubApp.AddFlags(fs)
}

func (o *PRCreationOptions) Finalize() error {
if err := o.GitHubApp.Validate(); err != nil {
return fmt.Errorf("failed to validate GitHub App options: %w", err)
}
if err := o.GitHubOptions.Validate(false); err != nil {
return err
}
Expand All @@ -39,7 +44,9 @@ func (o *PRCreationOptions) Finalize() error {
if err != nil {
return fmt.Errorf("failed to construct github client: %w", err)
}

if err := o.GitHubApp.Finalize(); err != nil {
return fmt.Errorf("failed to initialize GitHub App client: %w", err)
}
return nil
}

Expand Down Expand Up @@ -99,6 +106,25 @@ func SkipPRCreation() PrOption {
}
}

// formatAssigneeCC builds a /cc mention string from a comma-separated assignee list.
// Returns empty string when there are no assignees.
func formatAssigneeCC(assigneeCSV string) string {
if assigneeCSV == "" {
return ""
}
var mentions []string
for _, a := range strings.Split(assigneeCSV, ",") {
a = strings.TrimSpace(a)
if a != "" {
mentions = append(mentions, "@"+a)
}
}
if len(mentions) == 0 {
return ""
}
return "\n/cc " + strings.Join(mentions, ", ")
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment on lines +109 to +126
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new formatAssigneeCC helper function lacks unit test coverage. Given that this function fixes a bug (empty-assignee producing "/cc @"), it would be valuable to add unit tests covering edge cases such as empty strings, strings with only whitespace, single assignee, multiple assignees, and assignees with whitespace. This is particularly important because the codebase has comprehensive test coverage for similar utility functions.

Copilot uses AI. Check for mistakes.

// UpsertPR upserts a PR. The PRTitle must be alphanumeric except for spaces, as it will be used as the
// branchname on the bots fork.
func (o *PRCreationOptions) UpsertPR(localSourceDir, org, repo, branch, prTitle string, setters ...PrOption) error {
Expand Down Expand Up @@ -137,6 +163,7 @@ func (o *PRCreationOptions) UpsertPR(localSourceDir, org, repo, branch, prTitle
sourceBranchName := strings.ReplaceAll(strings.ToLower(prArgs.matchTitle), " ", "-")
// Fix for NO-ISSUE: title
sourceBranchName = strings.ReplaceAll(strings.ToLower(sourceBranchName), ":", "-")
origRepo := repo
o.GithubClient.SetMax404Retries(0)
if _, err := o.GithubClient.GetRepo(username, repo); err != nil {
// Somehow github.IsNotFound doesn't recognize this?
Expand Down Expand Up @@ -192,24 +219,29 @@ func (o *PRCreationOptions) UpsertPR(localSourceDir, org, repo, branch, prTitle
return nil
}

prBodyText := prArgs.prBody + formatAssigneeCC(prArgs.prAssignee)

labelsToAdd := prArgs.additionalLabels
if o.SelfApprove {
l.Infof("Self-aproving PR by adding the %q and %q labels", labels.Approved, labels.LGTM)
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a spelling error in this log message: "Self-aproving" should be "Self-approving".

Suggested change
l.Infof("Self-aproving PR by adding the %q and %q labels", labels.Approved, labels.LGTM)
l.Infof("Self-approving PR by adding the %q and %q labels", labels.Approved, labels.LGTM)

Copilot uses AI. Check for mistakes.
labelsToAdd = append(labelsToAdd, labels.Approved, labels.LGTM)
}

assignees := strings.Split(prArgs.prAssignee, ",")
for i, assignee := range assignees {
assignees[i] = "@" + assignee
if o.GitHubApp.Enabled() {
l.Info("Using GitHub App for PR creation")
head := username + ":" + sourceBranchName
if err := o.GitHubApp.UpsertPR(org, origRepo, branch, head, prTitle, prBodyText, labelsToAdd); err != nil {
return fmt.Errorf("failed to create PR via GitHub App: %w", err)
}
return nil
}
assigneeList := strings.Join(assignees, ", ")

if err := bumper.UpdatePullRequestWithLabels(
o.GithubClient,
org,
repo,
origRepo,
prTitle,
prArgs.prBody+"\n/cc "+assigneeList,
prBodyText,
username+":"+sourceBranchName,
branch,
sourceBranchName,
Expand Down
Loading