diff --git a/internal/forge/fake.go b/internal/forge/fake.go index 4e81135c6..cba6ebbe0 100644 --- a/internal/forge/fake.go +++ b/internal/forge/fake.go @@ -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 @@ -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() diff --git a/internal/forge/fake_test.go b/internal/forge/fake_test.go index 7825af8c2..d52e67821 100644 --- a/internal/forge/fake_test.go +++ b/internal/forge/fake_test.go @@ -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{ diff --git a/internal/forge/forge.go b/internal/forge/forge.go index 1ebe3eafe..4ba181872 100644 --- a/internal/forge/forge.go +++ b/internal/forge/forge.go @@ -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. @@ -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). + 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) diff --git a/internal/forge/github/github.go b/internal/forge/github/github.go index 7d671735a..188dc9348 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -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) @@ -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, }) } @@ -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)) diff --git a/internal/forge/github/github_test.go b/internal/forge/github/github_test.go index 8c977c50e..d1c63b9ba 100644 --- a/internal/forge/github/github_test.go +++ b/internal/forge/github/github_test.go @@ -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() @@ -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) { diff --git a/internal/forge/gitlab/methods_test.go b/internal/forge/gitlab/methods_test.go index b0021fd15..ad1f0d738 100644 --- a/internal/forge/gitlab/methods_test.go +++ b/internal/forge/gitlab/methods_test.go @@ -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() diff --git a/internal/forge/gitlab/mr.go b/internal/forge/gitlab/mr.go index 57015a93a..8842d301c 100644 --- a/internal/forge/gitlab/mr.go +++ b/internal/forge/gitlab/mr.go @@ -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) @@ -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, }) } @@ -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) diff --git a/internal/layers/commit.go b/internal/layers/commit.go index 783c4c1c2..58f550320 100644 --- a/internal/layers/commit.go +++ b/internal/layers/commit.go @@ -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", + "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. @@ -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) { + 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)) + 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 @@ -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") diff --git a/internal/layers/commit_test.go b/internal/layers/commit_test.go index 8b88579ba..1107a616b 100644 --- a/internal/layers/commit_test.go +++ b/internal/layers/commit_test.go @@ -675,3 +675,182 @@ func TestAdaptCommitMsg(t *testing.T) { assert.Equal(t, "ci: initialize fullsend per-repo installation\n\nSigned-off-by: bot", msg) }) } + +func TestCloseStaleScaffoldPRs_ClosesOnboardPR(t *testing.T) { + client := forge.NewFakeClient() + client.PullRequests = map[string][]forge.ChangeProposal{ + "acme/widget": { + {Number: 42, Title: "chore: connect to fullsend agent pipeline", Head: "fullsend/onboard", Base: "main", Author: "acme"}, + }, + } + printer, buf := newTestPrinter() + + closeStaleScaffoldPRs(context.Background(), client, printer, + "acme", "widget", "fullsend/scaffold-install", "acme") + + assert.Contains(t, client.ClosedProposals, 42) + assert.Contains(t, client.DeletedRefs, "acme/widget/heads/fullsend/onboard") + assert.Contains(t, buf.String(), "Closed stale scaffold PR #42") +} + +func TestCloseStaleScaffoldPRs_SkipsCurrentBranch(t *testing.T) { + client := forge.NewFakeClient() + client.PullRequests = map[string][]forge.ChangeProposal{ + "acme/widget": { + {Number: 10, Title: "scaffold PR", Head: "fullsend/scaffold-install", Base: "main", Author: "acme"}, + }, + } + printer, _ := newTestPrinter() + + closeStaleScaffoldPRs(context.Background(), client, printer, + "acme", "widget", "fullsend/scaffold-install", "acme") + + assert.Empty(t, client.ClosedProposals, "should not close the current branch's PR") +} + +func TestCloseStaleScaffoldPRs_SkipsUnrelatedPRs(t *testing.T) { + client := forge.NewFakeClient() + client.PullRequests = map[string][]forge.ChangeProposal{ + "acme/widget": { + {Number: 5, Title: "feat: add feature", Head: "feature/add-feature", Base: "main", Author: "acme"}, + }, + } + printer, _ := newTestPrinter() + + closeStaleScaffoldPRs(context.Background(), client, printer, + "acme", "widget", "fullsend/scaffold-install", "acme") + + assert.Empty(t, client.ClosedProposals, "should not close unrelated PRs") +} + +func TestCloseStaleScaffoldPRs_ListError(t *testing.T) { + client := forge.NewFakeClient() + client.Errors = map[string]error{ + "ListRepoPullRequests": fmt.Errorf("API rate limit"), + } + printer, buf := newTestPrinter() + + closeStaleScaffoldPRs(context.Background(), client, printer, + "acme", "widget", "fullsend/scaffold-install", "acme") + + assert.Empty(t, client.ClosedProposals) + assert.Contains(t, buf.String(), "Could not check for stale scaffold PRs") +} + +func TestCloseStaleScaffoldPRs_CloseError(t *testing.T) { + client := forge.NewFakeClient() + client.PullRequests = map[string][]forge.ChangeProposal{ + "acme/widget": { + {Number: 42, Title: "stale PR", Head: "fullsend/onboard", Base: "main", Author: "acme"}, + }, + } + client.Errors = map[string]error{ + "CloseChangeProposal": fmt.Errorf("forbidden"), + } + printer, buf := newTestPrinter() + + closeStaleScaffoldPRs(context.Background(), client, printer, + "acme", "widget", "fullsend/scaffold-install", "acme") + + assert.Empty(t, client.ClosedProposals, "should not record if close failed") + assert.Contains(t, buf.String(), "Could not close stale PR #42") +} + +func TestCommitScaffoldViaPR_ClosesStaleOnboardPR(t *testing.T) { + client := forge.NewFakeClient() + client.AuthenticatedUser = "acme" + client.PullRequests = map[string][]forge.ChangeProposal{ + "acme/widget": { + {Number: 99, Title: "chore: connect to fullsend", Head: "fullsend/onboard", Base: "main", Author: "acme"}, + }, + } + printer, buf := newTestPrinter() + + _, err := CommitScaffoldFiles(context.Background(), client, printer, + "acme", "widget", "main", "msg", "title", "body", testFiles, false, nil) + require.NoError(t, err) + + assert.Contains(t, client.ClosedProposals, 99, "stale onboard PR should be closed") + assert.Contains(t, buf.String(), "Closed stale scaffold PR #99") + + // Should still create its own branch and PR. + require.Len(t, client.CreatedBranches, 1) + assert.Equal(t, "acme/widget/fullsend/scaffold-install", client.CreatedBranches[0]) +} + +func TestCloseStaleScaffoldPRs_SkipsDifferentAuthor(t *testing.T) { + client := forge.NewFakeClient() + client.PullRequests = map[string][]forge.ChangeProposal{ + "acme/widget": { + {Number: 42, Title: "stale PR", Head: "fullsend/onboard", Base: "main", Author: "external-user"}, + }, + } + printer, _ := newTestPrinter() + + closeStaleScaffoldPRs(context.Background(), client, printer, + "acme", "widget", "fullsend/scaffold-install", "acme") + + assert.Empty(t, client.ClosedProposals, "should not close PRs from different author") +} + +func TestCloseStaleScaffoldPRs_SkipsEmptyAuthor(t *testing.T) { + client := forge.NewFakeClient() + client.PullRequests = map[string][]forge.ChangeProposal{ + "acme/widget": { + {Number: 42, Title: "stale PR", Head: "fullsend/onboard", Base: "main", Author: ""}, + }, + } + printer, _ := newTestPrinter() + + closeStaleScaffoldPRs(context.Background(), client, printer, + "acme", "widget", "fullsend/scaffold-install", "acme") + + assert.Empty(t, client.ClosedProposals, "should not close PRs with empty author (fail-closed)") +} + +func TestCloseStaleScaffoldPRs_DeleteRefError(t *testing.T) { + client := forge.NewFakeClient() + client.PullRequests = map[string][]forge.ChangeProposal{ + "acme/widget": { + {Number: 42, Title: "stale PR", Head: "fullsend/onboard", Base: "main", Author: "acme"}, + }, + } + client.Errors = map[string]error{ + "DeleteRef": fmt.Errorf("ref not found"), + } + printer, buf := newTestPrinter() + + closeStaleScaffoldPRs(context.Background(), client, printer, + "acme", "widget", "fullsend/scaffold-install", "acme") + + assert.Contains(t, client.ClosedProposals, 42, "PR should still be closed even if branch delete fails") + assert.Contains(t, buf.String(), "Could not delete stale branch fullsend/onboard") + assert.Contains(t, buf.String(), "Closed stale scaffold PR #42") +} + +func TestCommitScaffoldViaPR_SkipsStaleCleanupInForkPath(t *testing.T) { + client := forge.NewFakeClient() + client.AuthenticatedUser = "contributor" + client.ExistingForks = map[string]string{ + "acme/widget": "contributor", + } + client.PullRequests = map[string][]forge.ChangeProposal{ + "acme/widget": { + {Number: 99, Title: "chore: connect to fullsend", Head: "fullsend/onboard", Base: "main", Author: "acme"}, + }, + } + printer, _ := newTestPrinter() + + _, err := CommitScaffoldFiles(context.Background(), client, printer, + "acme", "widget", "main", "msg", "title", "body", testFiles, false, nil) + require.NoError(t, err) + + assert.Empty(t, client.ClosedProposals, "should not close upstream PRs when using fork path") +} + +func TestIsKnownScaffoldBranch(t *testing.T) { + assert.True(t, isKnownScaffoldBranch("fullsend/scaffold-install")) + assert.True(t, isKnownScaffoldBranch("fullsend/onboard")) + assert.False(t, isKnownScaffoldBranch("feature/add-stuff")) + assert.False(t, isKnownScaffoldBranch("fullsend/offboard")) +}