Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
13 changes: 13 additions & 0 deletions internal/forge/fake.go
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@ type FakeClient struct {
CommittedFiles []CommitFilesRecord
CommittedFilesToBranch []CommitFilesToBranchRecord
CreatedForks []string // "owner/repo"
ClosedProposals []int // PR numbers
DeletedComments []int // comment IDs
CreatedSchedules []PipelineSchedule
DeletedScheduleIDs []int64
Expand Down Expand Up @@ -856,6 +857,18 @@ func (f *FakeClient) ListRepoPullRequests(_ context.Context, owner, repo string)
return []ChangeProposal{}, nil
}

func (f *FakeClient) CloseChangeProposal(_ context.Context, _, _ string, number int) error {
f.mu.Lock()
defer f.mu.Unlock()

if e := f.err("CloseChangeProposal"); e != nil {
return e
}

f.ClosedProposals = append(f.ClosedProposals, number)
return nil
}

func (f *FakeClient) GetOrgPlan(_ context.Context, _ string) (string, error) {
f.mu.Lock()
defer f.mu.Unlock()
Expand Down
21 changes: 21 additions & 0 deletions internal/forge/fake_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1442,6 +1442,27 @@ func TestFakeClient_ListRepositoryFiles(t *testing.T) {
})
}

func TestFakeClient_CloseChangeProposal(t *testing.T) {
fc := NewFakeClient()
err := fc.CloseChangeProposal(context.Background(), "owner", "repo", 42)
require.NoError(t, err)
assert.Equal(t, []int{42}, fc.ClosedProposals)

err = fc.CloseChangeProposal(context.Background(), "owner", "repo", 99)
require.NoError(t, err)
assert.Equal(t, []int{42, 99}, fc.ClosedProposals)
}

func TestFakeClient_CloseChangeProposal_Error(t *testing.T) {
fc := NewFakeClient()
fc.Errors = map[string]error{
"CloseChangeProposal": fmt.Errorf("forbidden"),
}
err := fc.CloseChangeProposal(context.Background(), "owner", "repo", 1)
assert.ErrorContains(t, err, "forbidden")
assert.Empty(t, fc.ClosedProposals)
}

func TestFakeClient_ListRepositoryFiles_ConcurrentSafe(t *testing.T) {
fc := &FakeClient{
FileContents: map[string][]byte{
Expand Down
6 changes: 6 additions & 0 deletions internal/forge/forge.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ type ChangeProposal struct {
Number int
Head string
Base string
Author string // login of the user who opened the PR/MR
}

// PullRequestInfo carries branch/repo context for dispatch enrichment.
Expand Down Expand Up @@ -423,6 +424,11 @@ type Client interface {

ListRepoPullRequests(ctx context.Context, owner, repo string) ([]ChangeProposal, error)

// CloseChangeProposal closes an open pull request / merge request by
// number without merging it. This is used to clean up stale PRs
// (e.g., scaffold PRs left over from a previous install mode).
Comment thread
ggallen marked this conversation as resolved.
CloseChangeProposal(ctx context.Context, owner, repo string, number int) error

// Organization metadata
// GetOrgPlan returns the billing plan name for the org (e.g. "free", "team", "enterprise").
GetOrgPlan(ctx context.Context, org string) (string, error)
Expand Down
23 changes: 23 additions & 0 deletions internal/forge/github/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -1742,6 +1742,15 @@ func (c *LiveClient) ListRepoPullRequests(ctx context.Context, owner, repo strin
HTMLURL string `json:"html_url"`
Title string `json:"title"`
Number int `json:"number"`
Head struct {
Ref string `json:"ref"`
} `json:"head"`
Base struct {
Ref string `json:"ref"`
} `json:"base"`
User struct {
Login string `json:"login"`
} `json:"user"`
}
if err := decodeJSON(resp, &prs); err != nil {
return nil, fmt.Errorf("decode pull requests page %d: %w", page, err)
Expand All @@ -1752,6 +1761,9 @@ func (c *LiveClient) ListRepoPullRequests(ctx context.Context, owner, repo strin
URL: pr.HTMLURL,
Title: pr.Title,
Number: pr.Number,
Head: pr.Head.Ref,
Base: pr.Base.Ref,
Author: pr.User.Login,
})
}

Expand All @@ -1763,6 +1775,17 @@ func (c *LiveClient) ListRepoPullRequests(ctx context.Context, owner, repo strin
return result, nil
}

// CloseChangeProposal closes an open pull request without merging it.
func (c *LiveClient) CloseChangeProposal(ctx context.Context, owner, repo string, number int) error {
path := fmt.Sprintf("/repos/%s/%s/pulls/%d", owner, repo, number)
resp, err := c.patch(ctx, path, map[string]string{"state": "closed"})
if err != nil {
return fmt.Errorf("close pull request #%d: %w", number, err)
}
resp.Body.Close()
return nil
}

// GetOrgPlan returns the billing plan name for the org (e.g. "free", "team", "enterprise").
func (c *LiveClient) GetOrgPlan(ctx context.Context, org string) (string, error) {
resp, err := c.get(ctx, fmt.Sprintf("/orgs/%s", org))
Expand Down
42 changes: 40 additions & 2 deletions internal/forge/github/github_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -898,8 +898,22 @@ func TestListRepoPullRequests(t *testing.T) {
assert.Equal(t, "100", r.URL.Query().Get("per_page"))

json.NewEncoder(w).Encode([]map[string]any{
{"html_url": "https://github.com/owner/repo/pull/1", "title": "PR 1", "number": 1},
{"html_url": "https://github.com/owner/repo/pull/2", "title": "PR 2", "number": 2},
{
"html_url": "https://github.com/owner/repo/pull/1",
"title": "PR 1",
"number": 1,
"head": map[string]any{"ref": "feature-branch"},
"base": map[string]any{"ref": "main"},
"user": map[string]any{"login": "alice"},
},
{
"html_url": "https://github.com/owner/repo/pull/2",
"title": "PR 2",
"number": 2,
"head": map[string]any{"ref": "fix-branch"},
"base": map[string]any{"ref": "main"},
"user": map[string]any{"login": "bob"},
},
})
}))
defer srv.Close()
Expand All @@ -909,7 +923,31 @@ func TestListRepoPullRequests(t *testing.T) {
require.NoError(t, err)
require.Len(t, prs, 2)
assert.Equal(t, "PR 1", prs[0].Title)
assert.Equal(t, "feature-branch", prs[0].Head)
assert.Equal(t, "main", prs[0].Base)
assert.Equal(t, "alice", prs[0].Author)
assert.Equal(t, 2, prs[1].Number)
assert.Equal(t, "fix-branch", prs[1].Head)
assert.Equal(t, "bob", prs[1].Author)
}

func TestCloseChangeProposal(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
assert.Equal(t, "PATCH", r.Method)
assert.Equal(t, "/repos/owner/repo/pulls/42", r.URL.Path)

var body map[string]string
json.NewDecoder(r.Body).Decode(&body)
assert.Equal(t, "closed", body["state"])

w.WriteHeader(http.StatusOK)
json.NewEncoder(w).Encode(map[string]any{"number": 42, "state": "closed"})
}))
defer srv.Close()

client := newTestClient(t, srv)
err := client.CloseChangeProposal(context.Background(), "owner", "repo", 42)
require.NoError(t, err)
}

func TestGetAuthenticatedUser(t *testing.T) {
Expand Down
36 changes: 36 additions & 0 deletions internal/forge/gitlab/methods_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,42 @@ func TestListRepoPullRequests(t *testing.T) {
assert.Equal(t, "MR Two", mrs[1].Title)
}

func TestListRepoPullRequests_Author(t *testing.T) {
client, mux := setupTest(t)
ctx := context.Background()

mux.HandleFunc("/api/v4/projects/myorg%2Fmyrepo/merge_requests", func(w http.ResponseWriter, r *http.Request) {
writeJSON(t, w, http.StatusOK, []map[string]any{
{
"iid": 1,
"title": "MR One",
"web_url": "https://gitlab.com/myorg/myrepo/-/merge_requests/1",
"source_branch": "branch-1",
"target_branch": "main",
"author": map[string]any{"username": "alice"},
},
})
})

mrs, err := client.ListRepoPullRequests(ctx, "myorg", "myrepo")
require.NoError(t, err)
require.Len(t, mrs, 1)
assert.Equal(t, "alice", mrs[0].Author)
}

func TestCloseChangeProposal(t *testing.T) {
client, mux := setupTest(t)
ctx := context.Background()

mux.HandleFunc("/api/v4/projects/myorg%2Fmyrepo/merge_requests/5", func(w http.ResponseWriter, r *http.Request) {
assert.Equal(t, http.MethodPut, r.Method)
writeJSON(t, w, http.StatusOK, map[string]any{"iid": 5, "state": "closed"})
})

err := client.CloseChangeProposal(ctx, "myorg", "myrepo", 5)
require.NoError(t, err)
}

func TestGetPullRequestHeadSHA(t *testing.T) {
client, mux := setupTest(t)
ctx := context.Background()
Expand Down
15 changes: 15 additions & 0 deletions internal/forge/gitlab/mr.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,9 @@ func (c *LiveClient) ListRepoPullRequests(ctx context.Context, owner, repo strin
WebURL string `json:"web_url"`
SourceBranch string `json:"source_branch"`
TargetBranch string `json:"target_branch"`
Author struct {
Username string `json:"username"`
} `json:"author"`
}
if err := decodeJSON(resp, &mrs); err != nil {
return nil, fmt.Errorf("decode merge requests page %d: %w", page, err)
Expand All @@ -93,6 +96,7 @@ func (c *LiveClient) ListRepoPullRequests(ctx context.Context, owner, repo strin
Title: mr.Title,
Head: mr.SourceBranch,
Base: mr.TargetBranch,
Author: mr.Author.Username,
})
}

Expand Down Expand Up @@ -246,6 +250,17 @@ func (c *LiveClient) ListPullRequestFileDiffs(ctx context.Context, owner, repo s
return files, nil
}

// CloseChangeProposal closes an open merge request without merging it.
func (c *LiveClient) CloseChangeProposal(ctx context.Context, owner, repo string, number int) error {
path := fmt.Sprintf("/projects/%s/merge_requests/%d", projectPath(owner, repo), number)
resp, err := c.put(ctx, path, map[string]string{"state_event": "close"})
if err != nil {
return fmt.Errorf("close merge request !%d: %w", number, err)
}
resp.Body.Close()
return nil
}

// MergeChangeProposal merges a merge request by its IID.
func (c *LiveClient) MergeChangeProposal(ctx context.Context, owner, repo string, number int) error {
path := fmt.Sprintf("/projects/%s/merge_requests/%d/merge", projectPath(owner, repo), number)
Expand Down
85 changes: 85 additions & 0 deletions internal/layers/commit.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,15 @@ func CommitFilesViaPR(ctx context.Context, client forge.Client, printer *ui.Prin

const defaultScaffoldBranch = "fullsend/scaffold-install"

// knownScaffoldBranches lists all branch names that have been used to deliver
// scaffold files across different install modes. Per-org mode uses
// "fullsend/onboard" (via reconcile-repos.sh); per-repo mode uses
// "fullsend/scaffold-install" (via the Go CLI).
var knownScaffoldBranches = []string{
"fullsend/scaffold-install",
Comment thread
ggallen marked this conversation as resolved.
"fullsend/onboard",
}

// commitScaffoldViaPR creates a feature branch, commits files, and opens a PR.
// For non-owner users, it defaults to creating a fork and opening a cross-fork
// PR rather than pushing directly to the upstream repository.
Expand Down Expand Up @@ -167,6 +176,66 @@ func commitViaFork(ctx context.Context, client forge.Client, printer *ui.Printer
scaffoldBranch, defaultBranch, commitMsg, prTitle, prBody, files)
}

// closeStaleScaffoldPRs finds and closes open scaffold PRs from install modes
// other than the current one. When switching from per-org to per-repo (or vice
// versa), the old mode's scaffold PR may remain open with stale content (e.g.,
// referencing a deleted .fullsend repo). This function closes those PRs and
// deletes their head branches so they cannot be accidentally merged.
//
// Only PRs authored by authenticatedUser are closed (fail-closed: PRs with an
// empty author field are skipped), preventing accidental closure of PRs opened
// by external contributors on predictable branch names.
func closeStaleScaffoldPRs(ctx context.Context, client forge.Client, printer *ui.Printer,
upstreamOwner, upstreamRepo, currentBranch, authenticatedUser string) {

prs, err := client.ListRepoPullRequests(ctx, upstreamOwner, upstreamRepo)
if err != nil {
printer.StepWarn(fmt.Sprintf("Could not check for stale scaffold PRs: %v", err))
return
}

for _, pr := range prs {
if pr.Head == currentBranch || !isKnownScaffoldBranch(pr.Head) {
Comment thread
ggallen marked this conversation as resolved.
continue
}

// Only close PRs authored by the authenticated user to avoid
// silently closing PRs opened by external contributors on
// predictable scaffold branch names. Fail-closed: if the author
// field is empty (unexpected API response), skip rather than
// risk closing someone else's PR.
if pr.Author == "" || !strings.EqualFold(pr.Author, authenticatedUser) {
continue
}

printer.StepStart(fmt.Sprintf("Closing stale scaffold PR #%d (%s)", pr.Number, pr.Head))
if closeErr := client.CloseChangeProposal(ctx, upstreamOwner, upstreamRepo, pr.Number); closeErr != nil {
printer.StepWarn(fmt.Sprintf("Could not close stale PR #%d: %v", pr.Number, closeErr))
Comment thread
ggallen marked this conversation as resolved.
continue
}

// Best-effort branch cleanup — the PR is already closed, so a
// failure here just leaves a dangling ref.
refPath := "heads/" + pr.Head
if delErr := client.DeleteRef(ctx, upstreamOwner, upstreamRepo, refPath); delErr != nil {
printer.StepWarn(fmt.Sprintf("Could not delete stale branch %s: %v", pr.Head, delErr))
}

printer.StepDone(fmt.Sprintf("Closed stale scaffold PR #%d from previous install mode", pr.Number))
}
}

// isKnownScaffoldBranch reports whether branch is one of the well-known branch
// names used by scaffold installs (per-org or per-repo mode).
func isKnownScaffoldBranch(branch string) bool {
for _, b := range knownScaffoldBranches {
if b == branch {
return true
}
}
return false
}

// commitBranchAndPR creates a branch on targetOwner/targetRepo, commits files,
// and opens a PR against upstreamOwner/upstreamRepo. When target == upstream
// (same-repo PR), head is the branch name. When target != upstream (cross-fork
Expand All @@ -176,6 +245,22 @@ func commitBranchAndPR(ctx context.Context, client forge.Client, printer *ui.Pri
scaffoldBranch, defaultBranch, commitMsg, prTitle, prBody string,
files []forge.TreeFile) (bool, error) {

// Close stale scaffold PRs from a different install mode before
// creating or updating our own. This prevents merging a PR that
// references infrastructure from a mode that has been torn down.
//
// Only run in same-repo mode (target == upstream). In the fork path
// the caller's token likely lacks permission to close upstream PRs,
// which would produce unnecessary 403 warnings.
if strings.EqualFold(targetOwner, upstreamOwner) && strings.EqualFold(targetRepo, upstreamRepo) {
user, userErr := client.GetAuthenticatedUser(ctx)
if userErr != nil {
printer.StepWarn(fmt.Sprintf("Could not identify user for stale PR cleanup: %v", userErr))
} else {
closeStaleScaffoldPRs(ctx, client, printer, upstreamOwner, upstreamRepo, scaffoldBranch, user)
}
}

if branchErr := client.CreateBranch(ctx, targetOwner, targetRepo, scaffoldBranch); branchErr != nil {
if forge.IsForbidden(branchErr) {
printer.StepFail("Insufficient permissions to push to repository")
Expand Down
Loading
Loading