From 911d1429c8c351f1d75e42bcaefa80b5b46f3b95 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Thu, 11 Jun 2026 21:09:28 -0400 Subject: [PATCH 1/5] fix(cli): fall back to PR when default branch is protected When `fullsend github setup owner/repo` or `fullsend admin install owner/repo` encounters a protected default branch (422 on ref update), the CLI now falls back to creating a feature branch and opening a PR with the scaffold files instead of failing. Variables and secrets are set regardless of which path is taken. - Add ErrBranchProtected sentinel and CommitFilesToBranch to forge.Client - Refactor CommitFiles to detect 422 as branch protection failure - Add PR-based fallback in applyPerRepoScaffold Signed-off-by: Wayne Sun --- internal/cli/admin.go | 23 ++-- internal/cli/admin_test.go | 168 ++++++++++++++++++++++++++++++ internal/forge/fake.go | 72 +++++++++---- internal/forge/forge.go | 14 +++ internal/forge/github/github.go | 48 ++++++--- internal/layers/commit.go | 66 ++++++++++++ internal/layers/workflows.go | 25 ++--- internal/layers/workflows_test.go | 105 +++++++++++++++++++ 8 files changed, 459 insertions(+), 62 deletions(-) create mode 100644 internal/layers/commit.go diff --git a/internal/cli/admin.go b/internal/cli/admin.go index 9e8399e14..ec2027ccf 100644 --- a/internal/cli/admin.go +++ b/internal/cli/admin.go @@ -1023,22 +1023,17 @@ func applyPerRepoScaffold(ctx context.Context, client forge.Client, printer *ui. if err != nil { return fmt.Errorf("getting repo info: %w", err) } + commitMsg := fmt.Sprintf("chore: initialize fullsend-%s per-repo installation", version) printer.StepStart(fmt.Sprintf("Committing scaffold files to %s/%s (%s branch)", owner, repo, targetRepo.DefaultBranch)) - committed, err := client.CommitFiles(ctx, owner, repo, - fmt.Sprintf("chore: initialize fullsend-%s per-repo installation", version), files) - if err != nil { - printer.StepFail("Failed to commit scaffold files") - return fmt.Errorf("committing scaffold files: %w", err) - } - if committed { - noun := "files" - if len(files) == 1 { - noun = "file" - } - printer.StepDone(fmt.Sprintf("Pushed %d %s to %s", len(files), noun, targetRepo.DefaultBranch)) - } else { - printer.StepDone("Scaffold up to date") + prBody := fmt.Sprintf("This PR adds the fullsend scaffold files for per-repo installation.\n\n"+ + "The default branch (%s) has branch protection rules that prevent direct pushes, "+ + "so these files are delivered via PR instead.\n\n"+ + "Merge this PR to activate fullsend workflows.", targetRepo.DefaultBranch) + if err := layers.CommitScaffoldFiles(ctx, client, printer, + owner, repo, targetRepo.DefaultBranch, + commitMsg, "chore: initialize fullsend per-repo installation", prBody, files); err != nil { + return err } printer.StepStart("Configuring repository variables") diff --git a/internal/cli/admin_test.go b/internal/cli/admin_test.go index ea165c5a8..6a93bf124 100644 --- a/internal/cli/admin_test.go +++ b/internal/cli/admin_test.go @@ -1980,6 +1980,8 @@ func TestApplyPerRepoScaffold_CommitFilesError(t *testing.T) { "acme", "widget", files, nil, nil) require.Error(t, err) assert.Contains(t, err.Error(), "committing scaffold files") + assert.Empty(t, client.CreatedBranches, "should not attempt fallback for generic error") + assert.Empty(t, client.CreatedProposals, "should not attempt fallback for generic error") } func TestApplyPerRepoScaffold_Idempotent(t *testing.T) { @@ -2046,3 +2048,169 @@ func TestApplyPerRepoScaffold_CreateSecretError(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "setting repo secret") } + +func TestApplyPerRepoScaffold_ProtectedBranchFallback(t *testing.T) { + client := forge.NewFakeClient() + client.Repos = []forge.Repository{{FullName: "acme/widget", DefaultBranch: "main"}} + client.Errors["CommitFiles"] = fmt.Errorf("%w: github api: 422", forge.ErrBranchProtected) + var buf bytes.Buffer + printer := ui.New(&buf) + + files := []forge.TreeFile{ + {Path: ".github/workflows/fullsend.yaml", Content: []byte("workflow"), Mode: "100644"}, + } + repoVars := map[string]string{"K": "V"} + repoSecrets := map[string]string{"S": "secret"} + + err := applyPerRepoScaffold(context.Background(), client, printer, + "acme", "widget", files, repoVars, repoSecrets) + require.NoError(t, err) + + require.Len(t, client.CreatedBranches, 1) + assert.Equal(t, "acme/widget/fullsend/scaffold-install", client.CreatedBranches[0]) + + require.Len(t, client.CommittedFilesToBranch, 1) + assert.Equal(t, "fullsend/scaffold-install", client.CommittedFilesToBranch[0].Branch) + assert.Len(t, client.CommittedFilesToBranch[0].Files, 1) + + require.Len(t, client.CreatedProposals, 1) + assert.Contains(t, client.CreatedProposals[0].Title, "fullsend") + + output := buf.String() + assert.Contains(t, output, "protected") + assert.Contains(t, output, "PR #1") + assert.Contains(t, output, "Merge the PR") +} + +func TestApplyPerRepoScaffold_ProtectedBranch_ExistingBranch(t *testing.T) { + client := forge.NewFakeClient() + client.Repos = []forge.Repository{{FullName: "acme/widget", DefaultBranch: "main"}} + client.Errors["CommitFiles"] = fmt.Errorf("%w: github api: 422", forge.ErrBranchProtected) + client.Errors["CreateBranch"] = fmt.Errorf("already exists") + var buf bytes.Buffer + printer := ui.New(&buf) + + files := []forge.TreeFile{ + {Path: ".fullsend/config.yaml", Content: []byte("cfg"), Mode: "100644"}, + } + + err := applyPerRepoScaffold(context.Background(), client, printer, + "acme", "widget", files, nil, nil) + require.NoError(t, err) + + require.Len(t, client.CommittedFilesToBranch, 1, "should proceed despite branch existing") + require.Len(t, client.CreatedProposals, 1) +} + +func TestApplyPerRepoScaffold_ProtectedBranch_StillSetsVarsAndSecrets(t *testing.T) { + client := forge.NewFakeClient() + client.Repos = []forge.Repository{{FullName: "acme/widget", DefaultBranch: "main"}} + client.Errors["CommitFiles"] = fmt.Errorf("%w: github api: 422", forge.ErrBranchProtected) + printer := ui.New(&bytes.Buffer{}) + + files := []forge.TreeFile{ + {Path: ".fullsend/config.yaml", Content: []byte("cfg"), Mode: "100644"}, + } + repoVars := map[string]string{"FULLSEND_MINT_URL": "https://mint.example.run.app"} + repoSecrets := map[string]string{"FULLSEND_GCP_PROJECT_ID": "my-project"} + + err := applyPerRepoScaffold(context.Background(), client, printer, + "acme", "widget", files, repoVars, repoSecrets) + require.NoError(t, err) + + assert.Len(t, client.Variables, 1, "variables should be set even with PR fallback") + assert.Len(t, client.CreatedSecrets, 1, "secrets should be set even with PR fallback") +} + +func TestApplyPerRepoScaffold_ProtectedBranch_CreateBranchFails(t *testing.T) { + client := forge.NewFakeClient() + client.Repos = []forge.Repository{{FullName: "acme/widget", DefaultBranch: "main"}} + client.Errors["CommitFiles"] = fmt.Errorf("%w: github api: 422", forge.ErrBranchProtected) + client.Errors["CreateBranch"] = fmt.Errorf("forbidden") + printer := ui.New(&bytes.Buffer{}) + + files := []forge.TreeFile{ + {Path: ".fullsend/config.yaml", Content: []byte("cfg"), Mode: "100644"}, + } + + err := applyPerRepoScaffold(context.Background(), client, printer, + "acme", "widget", files, nil, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "creating scaffold branch") +} + +func TestApplyPerRepoScaffold_ProtectedBranch_CommitToBranchFails(t *testing.T) { + client := forge.NewFakeClient() + client.Repos = []forge.Repository{{FullName: "acme/widget", DefaultBranch: "main"}} + client.Errors["CommitFiles"] = fmt.Errorf("%w: github api: 422", forge.ErrBranchProtected) + client.Errors["CommitFilesToBranch"] = fmt.Errorf("server error") + printer := ui.New(&bytes.Buffer{}) + + files := []forge.TreeFile{ + {Path: ".fullsend/config.yaml", Content: []byte("cfg"), Mode: "100644"}, + } + + err := applyPerRepoScaffold(context.Background(), client, printer, + "acme", "widget", files, nil, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "committing scaffold files to branch") +} + +func TestApplyPerRepoScaffold_ProtectedBranch_CreatePRFails(t *testing.T) { + client := forge.NewFakeClient() + client.Repos = []forge.Repository{{FullName: "acme/widget", DefaultBranch: "main"}} + client.Errors["CommitFiles"] = fmt.Errorf("%w: github api: 422", forge.ErrBranchProtected) + client.Errors["CreateChangeProposal"] = fmt.Errorf("forbidden") + printer := ui.New(&bytes.Buffer{}) + + files := []forge.TreeFile{ + {Path: ".fullsend/config.yaml", Content: []byte("cfg"), Mode: "100644"}, + } + + err := applyPerRepoScaffold(context.Background(), client, printer, + "acme", "widget", files, nil, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "creating scaffold PR") +} + +func TestApplyPerRepoScaffold_ProtectedBranch_DuplicatePR(t *testing.T) { + client := forge.NewFakeClient() + client.Repos = []forge.Repository{{FullName: "acme/widget", DefaultBranch: "main"}} + client.Errors["CommitFiles"] = fmt.Errorf("%w: github api: 422", forge.ErrBranchProtected) + client.Errors["CreateChangeProposal"] = fmt.Errorf("already exists") + var buf bytes.Buffer + printer := ui.New(&buf) + + files := []forge.TreeFile{ + {Path: ".fullsend/config.yaml", Content: []byte("cfg"), Mode: "100644"}, + } + + err := applyPerRepoScaffold(context.Background(), client, printer, + "acme", "widget", files, nil, nil) + require.NoError(t, err) + + output := buf.String() + assert.Contains(t, output, "already exists") + assert.Contains(t, output, "Merge the PR") +} + +func TestApplyPerRepoScaffold_ProtectedBranch_BranchUpToDate(t *testing.T) { + client := forge.NewFakeClient() + client.Repos = []forge.Repository{{FullName: "acme/widget", DefaultBranch: "main"}} + client.Errors["CommitFiles"] = fmt.Errorf("%w: github api: 422", forge.ErrBranchProtected) + noChange := false + client.CommitFilesChanged = &noChange + var buf bytes.Buffer + printer := ui.New(&buf) + + files := []forge.TreeFile{ + {Path: ".fullsend/config.yaml", Content: []byte("cfg"), Mode: "100644"}, + } + + err := applyPerRepoScaffold(context.Background(), client, printer, + "acme", "widget", files, nil, nil) + require.NoError(t, err) + + assert.Empty(t, client.CreatedProposals, "should not create PR when branch files are unchanged") + assert.Contains(t, buf.String(), "up to date") +} diff --git a/internal/forge/fake.go b/internal/forge/fake.go index 69686d275..b0524a1b1 100644 --- a/internal/forge/fake.go +++ b/internal/forge/fake.go @@ -95,6 +95,12 @@ type CommitFilesRecord struct { Files []TreeFile } +// CommitFilesToBranchRecord records a CommitFilesToBranch call. +type CommitFilesToBranchRecord struct { + Owner, Repo, Branch, Message string + Files []TreeFile +} + // FakeClient is a thread-safe test double for forge.Client. // Pre-populate its fields to control return values, and inspect // recorder slices after the test to verify which calls were made. @@ -158,25 +164,26 @@ type FakeClient struct { Annotations []Annotation // Call recorders - CreatedRepos []Repository - CreatedFiles []FileRecord - CreatedBranches []string // "owner/repo/branch" - CreatedProposals []ChangeProposal - DeletedRepos []string // "owner/repo" - DeletedFiles []FileRecord - CreatedSecrets []SecretRecord - Variables []VariableRecord - DeletedOrgSecrets []string // "org/name" - CreatedOrgSecrets []OrgSecretRecord - CreatedOrgVariables []OrgVariableRecord - DeletedOrgVariables []string // "org/name" - CreatedIssues []CreatedIssueRecord - UpdatedComments []UpdatedCommentRecord - MinimizedComments []MinimizedCommentRecord - CreatedReviews []ReviewRecord - DismissedReviews []DismissedReviewRecord - CommittedFiles []CommitFilesRecord - DeletedComments []int // comment IDs + CreatedRepos []Repository + CreatedFiles []FileRecord + CreatedBranches []string // "owner/repo/branch" + CreatedProposals []ChangeProposal + DeletedRepos []string // "owner/repo" + DeletedFiles []FileRecord + CreatedSecrets []SecretRecord + Variables []VariableRecord + DeletedOrgSecrets []string // "org/name" + CreatedOrgSecrets []OrgSecretRecord + CreatedOrgVariables []OrgVariableRecord + DeletedOrgVariables []string // "org/name" + CreatedIssues []CreatedIssueRecord + UpdatedComments []UpdatedCommentRecord + MinimizedComments []MinimizedCommentRecord + CreatedReviews []ReviewRecord + DismissedReviews []DismissedReviewRecord + CommittedFiles []CommitFilesRecord + CommittedFilesToBranch []CommitFilesToBranchRecord + DeletedComments []int // comment IDs // internal counters proposalCounter int @@ -448,6 +455,33 @@ func (f *FakeClient) CommitFiles(_ context.Context, owner, repo, message string, return changed, nil } +func (f *FakeClient) CommitFilesToBranch(_ context.Context, owner, repo, branch, message string, files []TreeFile) (bool, error) { + f.mu.Lock() + defer f.mu.Unlock() + + if e := f.err("CommitFilesToBranch"); e != nil { + return false, e + } + + f.CommittedFilesToBranch = append(f.CommittedFilesToBranch, CommitFilesToBranchRecord{ + Owner: owner, + Repo: repo, + Branch: branch, + Message: message, + Files: files, + }) + + if f.FileContents == nil { + f.FileContents = make(map[string][]byte) + } + for _, file := range files { + f.FileContents[owner+"/"+repo+"/"+file.Path] = file.Content + } + + changed := f.CommitFilesChanged == nil || *f.CommitFilesChanged + return changed, nil +} + func (f *FakeClient) CreateBranch(_ context.Context, owner, repo, branchName string) error { f.mu.Lock() defer f.mu.Unlock() diff --git a/internal/forge/forge.go b/internal/forge/forge.go index 2bb135aba..9d2e5a5c6 100644 --- a/internal/forge/forge.go +++ b/internal/forge/forge.go @@ -24,6 +24,15 @@ func IsNotFound(err error) bool { return errors.Is(err, ErrNotFound) } +// ErrBranchProtected indicates that a ref update failed because the +// target branch has protection rules that prevent direct pushes. +var ErrBranchProtected = errors.New("branch is protected") + +// IsBranchProtected reports whether err indicates a branch protection failure. +func IsBranchProtected(err error) bool { + return errors.Is(err, ErrBranchProtected) +} + // Repository represents a repository on a git forge. type Repository struct { ID int64 @@ -186,6 +195,11 @@ type Client interface { // and it returns (false, nil). CommitFiles(ctx context.Context, owner, repo, message string, files []TreeFile) (committed bool, err error) + // CommitFilesToBranch atomically commits multiple files to a specific + // branch. Like CommitFiles, it is idempotent: if all files already + // have the expected content, no commit is created. + CommitFilesToBranch(ctx context.Context, owner, repo, branch, message string, files []TreeFile) (committed bool, err error) + // Branch operations CreateBranch(ctx context.Context, owner, repo, branchName string) error CreateFileOnBranch(ctx context.Context, owner, repo, branch, path, message string, content []byte) error diff --git a/internal/forge/github/github.go b/internal/forge/github/github.go index b048221d0..d1bbe45a6 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -599,12 +599,15 @@ func isTransientStatus(code int) bool { // CommitFiles atomically commits multiple files to the default branch // using the Git Trees/Blobs/Commits API. Returns (false, nil) when // all files already match the current tree (idempotent). +// +// Returns forge.ErrBranchProtected (wrapped) when the ref update fails +// with a 422, which indicates branch protection rules prevent direct pushes. func (c *LiveClient) CommitFiles(ctx context.Context, owner, repo, message string, files []forge.TreeFile) (bool, error) { if len(files) == 0 { return false, nil } - // 1. Get default branch name. + // Get default branch name. repoResp, err := c.get(ctx, fmt.Sprintf("/repos/%s/%s", owner, repo)) if err != nil { return false, fmt.Errorf("get repo: %w", err) @@ -616,12 +619,28 @@ func (c *LiveClient) CommitFiles(ctx context.Context, owner, repo, message strin return false, fmt.Errorf("decode repo info: %w", err) } - // 2. Get current commit SHA from the branch ref. - // Wrapped in retryOnTransient for freshly-created repos where the - // branch ref may not be materialized yet (async auto_init). + return c.commitFilesTo(ctx, owner, repo, repoInfo.DefaultBranch, message, files) +} + +// CommitFilesToBranch atomically commits multiple files to a specific +// branch. Like CommitFiles, it is idempotent. +func (c *LiveClient) CommitFilesToBranch(ctx context.Context, owner, repo, branch, message string, files []forge.TreeFile) (bool, error) { + if len(files) == 0 { + return false, nil + } + return c.commitFilesTo(ctx, owner, repo, branch, message, files) +} + +// commitFilesTo is the shared implementation for CommitFiles and +// CommitFilesToBranch. It commits files to the specified branch using +// the Git Trees/Blobs/Commits API. +func (c *LiveClient) commitFilesTo(ctx context.Context, owner, repo, branch, message string, files []forge.TreeFile) (bool, error) { + // 1. Get current commit SHA from the branch ref. + // Wrapped in retryOnTransient for freshly-created repos/branches where + // the ref may not be materialized yet (async auto_init). var commitSHA string if err := c.retryOnTransient(ctx, "get branch ref", func() error { - refResp, refErr := c.get(ctx, fmt.Sprintf("/repos/%s/%s/git/ref/heads/%s", owner, repo, repoInfo.DefaultBranch)) + refResp, refErr := c.get(ctx, fmt.Sprintf("/repos/%s/%s/git/ref/heads/%s", owner, repo, branch)) if refErr != nil { return fmt.Errorf("get branch ref: %w", refErr) } @@ -639,7 +658,7 @@ func (c *LiveClient) CommitFiles(ctx context.Context, owner, repo, message strin return false, err } - // 3. Get the current commit to find its tree SHA. + // 2. Get the current commit to find its tree SHA. cResp, err := c.get(ctx, fmt.Sprintf("/repos/%s/%s/git/commits/%s", owner, repo, commitSHA)) if err != nil { return false, fmt.Errorf("get commit: %w", err) @@ -654,7 +673,7 @@ func (c *LiveClient) CommitFiles(ctx context.Context, owner, repo, message strin } baseTreeSHA := commitObj.Tree.SHA - // 4. Get the full recursive tree to compare existing blobs. + // 3. Get the full recursive tree to compare existing blobs. treeResp, err := c.get(ctx, fmt.Sprintf("/repos/%s/%s/git/trees/%s?recursive=1", owner, repo, baseTreeSHA)) if err != nil { return false, fmt.Errorf("get tree: %w", err) @@ -683,7 +702,7 @@ func (c *LiveClient) CommitFiles(ctx context.Context, owner, repo, message strin existing[entry.Path] = blobInfo{sha: entry.SHA, mode: entry.Mode} } - // 5. Compute expected blob SHAs and filter to changed files. + // 4. Compute expected blob SHAs and filter to changed files. var changedEntries []map[string]string for _, f := range files { expectedSHA := blobSHA(f.Content) @@ -702,7 +721,7 @@ func (c *LiveClient) CommitFiles(ctx context.Context, owner, repo, message strin return false, nil } - // 6. Create new tree with base_tree + changed entries. + // 5. Create new tree with base_tree + changed entries. treePayload := map[string]any{ "base_tree": baseTreeSHA, "tree": changedEntries, @@ -718,7 +737,7 @@ func (c *LiveClient) CommitFiles(ctx context.Context, owner, repo, message strin return false, fmt.Errorf("decode new tree: %w", err) } - // 7. Create commit with new tree and old commit as parent. + // 6. Create commit with new tree and old commit as parent. commitPayload := map[string]any{ "message": message, "tree": newTree.SHA, @@ -735,12 +754,17 @@ func (c *LiveClient) CommitFiles(ctx context.Context, owner, repo, message strin return false, fmt.Errorf("decode new commit: %w", err) } - // 8. Update branch ref to point to new commit. + // 7. Update branch ref to point to new commit. + // A 422 here typically means branch protection rules prevent the push. refPayload := map[string]string{ "sha": newCommit.SHA, } - refUpdateResp, err := c.patch(ctx, fmt.Sprintf("/repos/%s/%s/git/refs/heads/%s", owner, repo, repoInfo.DefaultBranch), refPayload) + refUpdateResp, err := c.patch(ctx, fmt.Sprintf("/repos/%s/%s/git/refs/heads/%s", owner, repo, branch), refPayload) if err != nil { + var apiErr *APIError + if errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusUnprocessableEntity { + return false, fmt.Errorf("%w: %w", forge.ErrBranchProtected, err) + } return false, fmt.Errorf("update ref: %w", err) } refUpdateResp.Body.Close() diff --git a/internal/layers/commit.go b/internal/layers/commit.go new file mode 100644 index 000000000..1f3c6f6de --- /dev/null +++ b/internal/layers/commit.go @@ -0,0 +1,66 @@ +package layers + +import ( + "context" + "fmt" + "strings" + + "github.com/fullsend-ai/fullsend/internal/forge" + "github.com/fullsend-ai/fullsend/internal/ui" +) + +// CommitScaffoldFiles commits files to a repo's default branch. If the branch +// is protected, it falls back to creating a PR from a feature branch. +func CommitScaffoldFiles(ctx context.Context, client forge.Client, printer *ui.Printer, + owner, repo, defaultBranch, commitMsg, prTitle, prBody string, + files []forge.TreeFile) error { + + committed, err := client.CommitFiles(ctx, owner, repo, commitMsg, files) + if err != nil && forge.IsBranchProtected(err) { + printer.StepWarn("Default branch is protected — creating scaffold PR instead") + + const scaffoldBranch = "fullsend/scaffold-install" + if branchErr := client.CreateBranch(ctx, owner, repo, scaffoldBranch); branchErr != nil { + if !strings.Contains(branchErr.Error(), "already exists") { + printer.StepFail("Failed to create scaffold branch") + return fmt.Errorf("creating scaffold branch: %w", branchErr) + } + } + + branchCommitted, commitErr := client.CommitFilesToBranch(ctx, owner, repo, scaffoldBranch, commitMsg, files) + if commitErr != nil { + printer.StepFail("Failed to commit scaffold files to branch") + return fmt.Errorf("committing scaffold files to branch: %w", commitErr) + } + + if branchCommitted { + proposal, prErr := client.CreateChangeProposal(ctx, owner, repo, + prTitle, prBody, scaffoldBranch, defaultBranch) + if prErr != nil { + if !strings.Contains(prErr.Error(), "already exists") { + printer.StepFail("Failed to create scaffold PR") + return fmt.Errorf("creating scaffold PR: %w", prErr) + } + printer.StepDone("Scaffold PR already exists") + } else { + printer.StepDone(fmt.Sprintf("Created PR #%d: %s", proposal.Number, proposal.URL)) + } + printer.StepInfo("Merge the PR to activate fullsend workflows") + } else { + printer.StepDone("Scaffold branch up to date") + } + } else if err != nil { + printer.StepFail("Failed to commit scaffold files") + return fmt.Errorf("committing scaffold files: %w", err) + } else if committed { + noun := "files" + if len(files) == 1 { + noun = "file" + } + printer.StepDone(fmt.Sprintf("Pushed %d %s to %s", len(files), noun, defaultBranch)) + } else { + printer.StepDone("Scaffold up to date") + } + + return nil +} diff --git a/internal/layers/workflows.go b/internal/layers/workflows.go index ce21d6c90..f736ac442 100644 --- a/internal/layers/workflows.go +++ b/internal/layers/workflows.go @@ -113,23 +113,14 @@ func (l *WorkflowsLayer) Install(ctx context.Context) error { } l.ui.StepStart(fmt.Sprintf("Committing scaffold files to %s/%s (%s branch)", l.org, forge.ConfigRepoName, cfgRepo.DefaultBranch)) - committed, err := l.client.CommitFiles(ctx, l.org, forge.ConfigRepoName, - fmt.Sprintf("chore: update fullsend-%s scaffold", l.version), files) - if err != nil { - l.ui.StepFail("Failed to commit scaffold files") - return fmt.Errorf("committing scaffold files: %w", err) - } - if committed { - noun := "files" - if len(files) == 1 { - noun = "file" - } - l.ui.StepDone(fmt.Sprintf("Pushed %d %s to %s", len(files), noun, cfgRepo.DefaultBranch)) - } else { - l.ui.StepDone("Scaffold up to date") - } - - return nil + commitMsg := fmt.Sprintf("chore: update fullsend-%s scaffold", l.version) + prBody := fmt.Sprintf("This PR adds the fullsend scaffold files to the %s config repo.\n\n"+ + "The default branch (%s) has branch protection rules that prevent direct pushes, "+ + "so these files are delivered via PR instead.\n\n"+ + "Merge this PR to activate fullsend workflows.", forge.ConfigRepoName, cfgRepo.DefaultBranch) + return CommitScaffoldFiles(ctx, l.client, l.ui, + l.org, forge.ConfigRepoName, cfgRepo.DefaultBranch, + commitMsg, commitMsg, prBody, files) } // Uninstall is a no-op. Workflow files are removed when the config repo diff --git a/internal/layers/workflows_test.go b/internal/layers/workflows_test.go index 5c769e6be..d3dfdb29c 100644 --- a/internal/layers/workflows_test.go +++ b/internal/layers/workflows_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "errors" + "fmt" "strings" "testing" @@ -126,6 +127,110 @@ func TestWorkflowsLayer_Install_ManagedHeaders(t *testing.T) { } } +func TestWorkflowsLayer_Install_ProtectedBranchFallback(t *testing.T) { + client := forge.NewFakeClient() + client.Repos = []forge.Repository{{FullName: "test-org/.fullsend", DefaultBranch: "main"}} + client.Errors["CommitFiles"] = fmt.Errorf("%w: github api: 422", forge.ErrBranchProtected) + layer, buf := newWorkflowsLayer(t, client) + + err := layer.Install(context.Background()) + require.NoError(t, err) + + require.Len(t, client.CreatedBranches, 1) + assert.Equal(t, "test-org/.fullsend/fullsend/scaffold-install", client.CreatedBranches[0]) + + require.Len(t, client.CommittedFilesToBranch, 1) + assert.Equal(t, "fullsend/scaffold-install", client.CommittedFilesToBranch[0].Branch) + + require.Len(t, client.CreatedProposals, 1) + assert.Contains(t, client.CreatedProposals[0].Title, "fullsend") + + output := buf.String() + assert.Contains(t, output, "protected") + assert.Contains(t, output, "PR #1") + assert.Contains(t, output, "Merge the PR") +} + +func TestWorkflowsLayer_Install_ProtectedBranch_ExistingBranch(t *testing.T) { + client := forge.NewFakeClient() + client.Repos = []forge.Repository{{FullName: "test-org/.fullsend", DefaultBranch: "main"}} + client.Errors["CommitFiles"] = fmt.Errorf("%w: github api: 422", forge.ErrBranchProtected) + client.Errors["CreateBranch"] = fmt.Errorf("already exists") + layer, _ := newWorkflowsLayer(t, client) + + err := layer.Install(context.Background()) + require.NoError(t, err) + + require.Len(t, client.CommittedFilesToBranch, 1, "should proceed despite branch existing") + require.Len(t, client.CreatedProposals, 1) +} + +func TestWorkflowsLayer_Install_ProtectedBranch_CreateBranchFails(t *testing.T) { + client := forge.NewFakeClient() + client.Repos = []forge.Repository{{FullName: "test-org/.fullsend", DefaultBranch: "main"}} + client.Errors["CommitFiles"] = fmt.Errorf("%w: github api: 422", forge.ErrBranchProtected) + client.Errors["CreateBranch"] = fmt.Errorf("forbidden") + layer, _ := newWorkflowsLayer(t, client) + + err := layer.Install(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "creating scaffold branch") +} + +func TestWorkflowsLayer_Install_ProtectedBranch_CommitToBranchFails(t *testing.T) { + client := forge.NewFakeClient() + client.Repos = []forge.Repository{{FullName: "test-org/.fullsend", DefaultBranch: "main"}} + client.Errors["CommitFiles"] = fmt.Errorf("%w: github api: 422", forge.ErrBranchProtected) + client.Errors["CommitFilesToBranch"] = fmt.Errorf("server error") + layer, _ := newWorkflowsLayer(t, client) + + err := layer.Install(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "committing scaffold files to branch") +} + +func TestWorkflowsLayer_Install_ProtectedBranch_CreatePRFails(t *testing.T) { + client := forge.NewFakeClient() + client.Repos = []forge.Repository{{FullName: "test-org/.fullsend", DefaultBranch: "main"}} + client.Errors["CommitFiles"] = fmt.Errorf("%w: github api: 422", forge.ErrBranchProtected) + client.Errors["CreateChangeProposal"] = fmt.Errorf("forbidden") + layer, _ := newWorkflowsLayer(t, client) + + err := layer.Install(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "creating scaffold PR") +} + +func TestWorkflowsLayer_Install_ProtectedBranch_DuplicatePR(t *testing.T) { + client := forge.NewFakeClient() + client.Repos = []forge.Repository{{FullName: "test-org/.fullsend", DefaultBranch: "main"}} + client.Errors["CommitFiles"] = fmt.Errorf("%w: github api: 422", forge.ErrBranchProtected) + client.Errors["CreateChangeProposal"] = fmt.Errorf("already exists") + layer, buf := newWorkflowsLayer(t, client) + + err := layer.Install(context.Background()) + require.NoError(t, err) + + output := buf.String() + assert.Contains(t, output, "already exists") + assert.Contains(t, output, "Merge the PR") +} + +func TestWorkflowsLayer_Install_ProtectedBranch_BranchUpToDate(t *testing.T) { + client := forge.NewFakeClient() + client.Repos = []forge.Repository{{FullName: "test-org/.fullsend", DefaultBranch: "main"}} + client.Errors["CommitFiles"] = fmt.Errorf("%w: github api: 422", forge.ErrBranchProtected) + noChange := false + client.CommitFilesChanged = &noChange + layer, buf := newWorkflowsLayer(t, client) + + err := layer.Install(context.Background()) + require.NoError(t, err) + + assert.Empty(t, client.CreatedProposals, "should not create PR when branch files are unchanged") + assert.Contains(t, buf.String(), "up to date") +} + func TestWorkflowsLayer_Install_Error(t *testing.T) { client := &forge.FakeClient{ Repos: []forge.Repository{{ From 45331342d79cf6ee8b31a061c00e30c54d2da383 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Mon, 15 Jun 2026 10:34:52 -0400 Subject: [PATCH 2/5] fix(cli): tighten error handling in scaffold branch fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add ErrAlreadyExists sentinel to forge package and wire it through APIError.Unwrap for 422 responses containing "already exists" - Replace fragile strings.Contains checks in CommitScaffoldFiles with forge.IsAlreadyExists (errors.Is) - Narrow 422 → ErrBranchProtected detection to only match protection- related messages, avoiding false positives on other 422 errors - Detect ErrBranchProtected on scaffold branch commits and surface a specific error message instead of a generic failure - Use a version-free PR title for org install scaffold PRs so the title stays accurate across version bumps Assisted-by: Claude (fix), Gemini (review), Codex (review) Signed-off-by: Wayne Sun --- internal/cli/admin_test.go | 4 ++-- internal/forge/forge.go | 8 ++++++++ internal/forge/github/github.go | 25 +++++++++++++++++++++++-- internal/layers/commit.go | 9 ++++++--- internal/layers/workflows.go | 3 ++- internal/layers/workflows_test.go | 4 ++-- 6 files changed, 43 insertions(+), 10 deletions(-) diff --git a/internal/cli/admin_test.go b/internal/cli/admin_test.go index 6a93bf124..a88f67970 100644 --- a/internal/cli/admin_test.go +++ b/internal/cli/admin_test.go @@ -2086,7 +2086,7 @@ func TestApplyPerRepoScaffold_ProtectedBranch_ExistingBranch(t *testing.T) { client := forge.NewFakeClient() client.Repos = []forge.Repository{{FullName: "acme/widget", DefaultBranch: "main"}} client.Errors["CommitFiles"] = fmt.Errorf("%w: github api: 422", forge.ErrBranchProtected) - client.Errors["CreateBranch"] = fmt.Errorf("already exists") + client.Errors["CreateBranch"] = fmt.Errorf("branch: %w", forge.ErrAlreadyExists) var buf bytes.Buffer printer := ui.New(&buf) @@ -2177,7 +2177,7 @@ func TestApplyPerRepoScaffold_ProtectedBranch_DuplicatePR(t *testing.T) { client := forge.NewFakeClient() client.Repos = []forge.Repository{{FullName: "acme/widget", DefaultBranch: "main"}} client.Errors["CommitFiles"] = fmt.Errorf("%w: github api: 422", forge.ErrBranchProtected) - client.Errors["CreateChangeProposal"] = fmt.Errorf("already exists") + client.Errors["CreateChangeProposal"] = fmt.Errorf("pr: %w", forge.ErrAlreadyExists) var buf bytes.Buffer printer := ui.New(&buf) diff --git a/internal/forge/forge.go b/internal/forge/forge.go index 9d2e5a5c6..b6b295aca 100644 --- a/internal/forge/forge.go +++ b/internal/forge/forge.go @@ -24,6 +24,14 @@ func IsNotFound(err error) bool { return errors.Is(err, ErrNotFound) } +// ErrAlreadyExists indicates that the resource already exists. +var ErrAlreadyExists = errors.New("already exists") + +// IsAlreadyExists reports whether err indicates a duplicate resource. +func IsAlreadyExists(err error) bool { + return errors.Is(err, ErrAlreadyExists) +} + // ErrBranchProtected indicates that a ref update failed because the // target branch has protection rules that prevent direct pushes. var ErrBranchProtected = errors.New("branch is protected") diff --git a/internal/forge/github/github.go b/internal/forge/github/github.go index d1bbe45a6..8fe147e73 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -71,11 +71,14 @@ func (e *APIError) Error() string { return s } -// Unwrap returns forge.ErrNotFound for 404 errors, enabling errors.Is checks. +// Unwrap returns sentinel errors for well-known API responses. func (e *APIError) Unwrap() error { if e.StatusCode == http.StatusNotFound { return forge.ErrNotFound } + if e.StatusCode == http.StatusUnprocessableEntity && isAlreadyExistsError(e) { + return forge.ErrAlreadyExists + } return nil } @@ -762,7 +765,7 @@ func (c *LiveClient) commitFilesTo(ctx context.Context, owner, repo, branch, mes refUpdateResp, err := c.patch(ctx, fmt.Sprintf("/repos/%s/%s/git/refs/heads/%s", owner, repo, branch), refPayload) if err != nil { var apiErr *APIError - if errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusUnprocessableEntity { + if errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusUnprocessableEntity && isBranchProtectionError(apiErr) { return false, fmt.Errorf("%w: %w", forge.ErrBranchProtected, err) } return false, fmt.Errorf("update ref: %w", err) @@ -772,6 +775,24 @@ func (c *LiveClient) commitFilesTo(ctx context.Context, owner, repo, branch, mes return true, nil } +// isBranchProtectionError checks whether a 422 APIError indicates branch +// protection rather than another validation failure (e.g. non-fast-forward). +func isBranchProtectionError(apiErr *APIError) bool { + msg := strings.ToLower(apiErr.Message) + for _, d := range apiErr.Errors { + msg += " " + strings.ToLower(d.Message) + } + return strings.Contains(msg, "protected") || strings.Contains(msg, "required status") || strings.Contains(msg, "required review") +} + +func isAlreadyExistsError(apiErr *APIError) bool { + msg := strings.ToLower(apiErr.Message) + for _, d := range apiErr.Errors { + msg += " " + strings.ToLower(d.Message) + } + return strings.Contains(msg, "already exists") +} + // blobSHA computes the Git blob object SHA-1 for the given content. func blobSHA(content []byte) string { h := sha1.New() diff --git a/internal/layers/commit.go b/internal/layers/commit.go index 1f3c6f6de..1a3979834 100644 --- a/internal/layers/commit.go +++ b/internal/layers/commit.go @@ -3,7 +3,6 @@ package layers import ( "context" "fmt" - "strings" "github.com/fullsend-ai/fullsend/internal/forge" "github.com/fullsend-ai/fullsend/internal/ui" @@ -21,7 +20,7 @@ func CommitScaffoldFiles(ctx context.Context, client forge.Client, printer *ui.P const scaffoldBranch = "fullsend/scaffold-install" if branchErr := client.CreateBranch(ctx, owner, repo, scaffoldBranch); branchErr != nil { - if !strings.Contains(branchErr.Error(), "already exists") { + if !forge.IsAlreadyExists(branchErr) { printer.StepFail("Failed to create scaffold branch") return fmt.Errorf("creating scaffold branch: %w", branchErr) } @@ -29,6 +28,10 @@ func CommitScaffoldFiles(ctx context.Context, client forge.Client, printer *ui.P branchCommitted, commitErr := client.CommitFilesToBranch(ctx, owner, repo, scaffoldBranch, commitMsg, files) if commitErr != nil { + if forge.IsBranchProtected(commitErr) { + printer.StepFail("Scaffold branch is also protected — cannot commit") + return fmt.Errorf("scaffold branch %q is protected; configure branch protection to allow pushes to scaffold branches: %w", scaffoldBranch, commitErr) + } printer.StepFail("Failed to commit scaffold files to branch") return fmt.Errorf("committing scaffold files to branch: %w", commitErr) } @@ -37,7 +40,7 @@ func CommitScaffoldFiles(ctx context.Context, client forge.Client, printer *ui.P proposal, prErr := client.CreateChangeProposal(ctx, owner, repo, prTitle, prBody, scaffoldBranch, defaultBranch) if prErr != nil { - if !strings.Contains(prErr.Error(), "already exists") { + if !forge.IsAlreadyExists(prErr) { printer.StepFail("Failed to create scaffold PR") return fmt.Errorf("creating scaffold PR: %w", prErr) } diff --git a/internal/layers/workflows.go b/internal/layers/workflows.go index f736ac442..a36826c25 100644 --- a/internal/layers/workflows.go +++ b/internal/layers/workflows.go @@ -114,13 +114,14 @@ func (l *WorkflowsLayer) Install(ctx context.Context) error { l.ui.StepStart(fmt.Sprintf("Committing scaffold files to %s/%s (%s branch)", l.org, forge.ConfigRepoName, cfgRepo.DefaultBranch)) commitMsg := fmt.Sprintf("chore: update fullsend-%s scaffold", l.version) + prTitle := "chore: add fullsend scaffold files" prBody := fmt.Sprintf("This PR adds the fullsend scaffold files to the %s config repo.\n\n"+ "The default branch (%s) has branch protection rules that prevent direct pushes, "+ "so these files are delivered via PR instead.\n\n"+ "Merge this PR to activate fullsend workflows.", forge.ConfigRepoName, cfgRepo.DefaultBranch) return CommitScaffoldFiles(ctx, l.client, l.ui, l.org, forge.ConfigRepoName, cfgRepo.DefaultBranch, - commitMsg, commitMsg, prBody, files) + commitMsg, prTitle, prBody, files) } // Uninstall is a no-op. Workflow files are removed when the config repo diff --git a/internal/layers/workflows_test.go b/internal/layers/workflows_test.go index d3dfdb29c..bbec4c977 100644 --- a/internal/layers/workflows_test.go +++ b/internal/layers/workflows_test.go @@ -155,7 +155,7 @@ func TestWorkflowsLayer_Install_ProtectedBranch_ExistingBranch(t *testing.T) { client := forge.NewFakeClient() client.Repos = []forge.Repository{{FullName: "test-org/.fullsend", DefaultBranch: "main"}} client.Errors["CommitFiles"] = fmt.Errorf("%w: github api: 422", forge.ErrBranchProtected) - client.Errors["CreateBranch"] = fmt.Errorf("already exists") + client.Errors["CreateBranch"] = fmt.Errorf("branch: %w", forge.ErrAlreadyExists) layer, _ := newWorkflowsLayer(t, client) err := layer.Install(context.Background()) @@ -205,7 +205,7 @@ func TestWorkflowsLayer_Install_ProtectedBranch_DuplicatePR(t *testing.T) { client := forge.NewFakeClient() client.Repos = []forge.Repository{{FullName: "test-org/.fullsend", DefaultBranch: "main"}} client.Errors["CommitFiles"] = fmt.Errorf("%w: github api: 422", forge.ErrBranchProtected) - client.Errors["CreateChangeProposal"] = fmt.Errorf("already exists") + client.Errors["CreateChangeProposal"] = fmt.Errorf("PR: %w", forge.ErrAlreadyExists) layer, buf := newWorkflowsLayer(t, client) err := layer.Install(context.Background()) From ccd047033ac194d1e3210a01e7f18f5e0c8ba959 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Mon, 15 Jun 2026 15:19:54 -0400 Subject: [PATCH 3/5] docs: document protected branch fallback in forge ADR and install guides Update ADR-0005 to acknowledge sentinel errors (ErrBranchProtected, ErrAlreadyExists) and CommitFilesToBranch. Add protected-branch fallback flow to Phase 5 in cli-internals.md. Note PR-based scaffold delivery in installation.md for protected default branches. Assisted-by: Claude Signed-off-by: Wayne Sun --- docs/ADRs/0005-forge-abstraction-layer.md | 2 ++ docs/guides/dev/cli-internals.md | 5 +++++ docs/reference/installation.md | 4 ++++ 3 files changed, 11 insertions(+) diff --git a/docs/ADRs/0005-forge-abstraction-layer.md b/docs/ADRs/0005-forge-abstraction-layer.md index eed29505d..39da8c5b6 100644 --- a/docs/ADRs/0005-forge-abstraction-layer.md +++ b/docs/ADRs/0005-forge-abstraction-layer.md @@ -34,3 +34,5 @@ No code outside `internal/forge/` imports forge-specific packages directly. - Forge-neutral naming occasionally feels awkward (e.g., `ChangeProposal`), but prevents GitHub-centric thinking from leaking into the model. - The interface will grow as new operations are needed; keeping it cohesive requires discipline. - The `FakeClient` enables deterministic testing of every layer without network calls. +- Sentinel errors (`ErrNotFound`, `ErrBranchProtected`, `ErrAlreadyExists`) with `errors.Is()` helpers provide forge-agnostic error classification. Forge implementations wrap these sentinels via `Unwrap()` so callers never inspect HTTP status codes or provider-specific messages directly. +- `CommitFilesToBranch` complements `CommitFiles` (default branch) by targeting a specific branch, enabling the protected-branch fallback path where scaffold files are committed to a feature branch and delivered via PR. diff --git a/docs/guides/dev/cli-internals.md b/docs/guides/dev/cli-internals.md index 80dcb82bb..ba1ae404f 100644 --- a/docs/guides/dev/cli-internals.md +++ b/docs/guides/dev/cli-internals.md @@ -175,6 +175,11 @@ Both per-org and per-repo modes share the same core pipeline. The code follows t │ │ Phase 5: Write scaffold + config files │ │ │ │ │ │ │ │ Both modes: write workflow files + customized/ dirs │ │ +│ │ CommitScaffoldFiles() handles protected-branch fallback: │ │ +│ │ 1. Try CommitFiles (default branch) │ │ +│ │ 2. If ErrBranchProtected → create feature branch │ │ +│ │ 3. CommitFilesToBranch on feature branch │ │ +│ │ 4. Open PR back to default branch │ │ │ │ ┌──────────────────────────────────────────┐ │ │ │ │ │ Per-org: create .fullsend config repo │ │ │ │ │ │ push reusable workflows │ │ │ diff --git a/docs/reference/installation.md b/docs/reference/installation.md index 6b3e585bd..a1364a4f9 100644 --- a/docs/reference/installation.md +++ b/docs/reference/installation.md @@ -121,6 +121,8 @@ The `--inference-region` flag defaults to `global` for the broadest model availa See [Setting up with pre-provisioned infrastructure](github-setup.md) for the full `github setup` reference, including per-repo mode, `--skip-app-setup`, and day-2 operations. +> **Protected default branch:** If the `.fullsend` config repo or target repo has branch protection rules that prevent direct pushes, the installer automatically falls back to creating a PR with the scaffold files instead of pushing directly. Merge the scaffold PR to complete setup. + ### Step 4: Merge enrollment PRs If you enrolled repositories during setup, the installer dispatches a workflow that creates an enrollment PR in each enrolled repo. These PRs add a shim workflow (`.github/workflows/fullsend.yaml`) that wires events to the agent pipeline. @@ -214,6 +216,8 @@ During installation, you'll be prompted to choose repository enrollment: The installer creates the `.fullsend` config repo as **public** by default. This is required for cross-repo `workflow_call` to work with enrolled repos of any visibility (public, private, or internal) across all GitHub plan tiers. If an admin later makes `.fullsend` private, only other private repos in the org will be able to trigger agent workflows — public and internal repos will fail silently. +If the default branch of the `.fullsend` config repo has branch protection rules, the installer creates a PR with the scaffold files instead of pushing directly. Merge the scaffold PR to complete setup. + If the installer fails partway through, run `fullsend admin uninstall "$ORG_NAME"` to clean up before retrying. The uninstall preflight will prompt you to add the `delete_repo` scope if it is missing. Set the variables for your environment: From 4043f1d6f59a77f14c218af531229f03c7cea067 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Mon, 15 Jun 2026 16:34:12 -0400 Subject: [PATCH 4/5] test(forge): add table-driven tests for API error classification Test isBranchProtectionError, isAlreadyExistsError, and APIError.Unwrap against realistic GitHub API payloads including protected branch push rejections, required status checks, required reviews, reference already-exists, PR already-exists (custom code), non-fast-forward (negative case), and unrelated validation errors. Assisted-by: Claude Signed-off-by: Wayne Sun --- internal/forge/github/github_test.go | 186 +++++++++++++++++++++++++++ 1 file changed, 186 insertions(+) diff --git a/internal/forge/github/github_test.go b/internal/forge/github/github_test.go index 2d302159a..214c23dba 100644 --- a/internal/forge/github/github_test.go +++ b/internal/forge/github/github_test.go @@ -630,6 +630,192 @@ func TestAPIError_ErrorStringWithDetails(t *testing.T) { assert.Contains(t, err.Error(), "name already exists on this account") } +func TestIsBranchProtectionError(t *testing.T) { + tests := []struct { + name string + apiErr *APIError + want bool + }{ + { + name: "protected branch push rejected", + apiErr: &APIError{ + StatusCode: 422, + Message: "Update is not a fast forward", + Errors: []APIErrorDetail{ + {Message: "Protected branch update failed for refs/heads/main."}, + }, + }, + want: true, + }, + { + name: "required status check failing", + apiErr: &APIError{ + StatusCode: 422, + Message: "Update is not a fast forward", + Errors: []APIErrorDetail{ + {Message: "Required status check 'ci-build' is failing"}, + }, + }, + want: true, + }, + { + name: "required review", + apiErr: &APIError{ + StatusCode: 422, + Message: "Validation Failed", + Errors: []APIErrorDetail{ + {Message: "Required review from a code owner is not satisfied"}, + }, + }, + want: true, + }, + { + name: "protection in top-level message", + apiErr: &APIError{StatusCode: 422, Message: "Protected branch 'main' does not allow direct pushes"}, + want: true, + }, + { + name: "non-fast-forward without protection", + apiErr: &APIError{StatusCode: 422, Message: "Update is not a fast forward"}, + want: false, + }, + { + name: "reference already exists", + apiErr: &APIError{StatusCode: 422, Message: "Reference already exists"}, + want: false, + }, + { + name: "validation failed for unrelated reason", + apiErr: &APIError{ + StatusCode: 422, + Message: "Validation Failed", + Errors: []APIErrorDetail{ + {Resource: "PullRequest", Code: "custom", Message: "No commits between main and main"}, + }, + }, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, isBranchProtectionError(tt.apiErr)) + }) + } +} + +func TestIsAlreadyExistsError(t *testing.T) { + tests := []struct { + name string + apiErr *APIError + want bool + }{ + { + name: "reference already exists", + apiErr: &APIError{StatusCode: 422, Message: "Reference already exists"}, + want: true, + }, + { + name: "PR already exists via custom code", + apiErr: &APIError{ + StatusCode: 422, + Message: "Validation Failed", + Errors: []APIErrorDetail{ + {Resource: "PullRequest", Code: "custom", Message: "A pull request already exists for user:branch."}, + }, + }, + want: true, + }, + { + name: "repo name already exists on account", + apiErr: &APIError{ + StatusCode: 422, + Message: "Validation Failed", + Errors: []APIErrorDetail{ + {Resource: "Repository", Field: "name", Code: "custom", Message: "name already exists on this account"}, + }, + }, + want: true, + }, + { + name: "non-fast-forward", + apiErr: &APIError{StatusCode: 422, Message: "Update is not a fast forward"}, + want: false, + }, + { + name: "branch protection", + apiErr: &APIError{ + StatusCode: 422, + Message: "Update is not a fast forward", + Errors: []APIErrorDetail{ + {Message: "Protected branch update failed for refs/heads/main."}, + }, + }, + want: false, + }, + { + name: "not found", + apiErr: &APIError{StatusCode: 404, Message: "Not Found"}, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, isAlreadyExistsError(tt.apiErr)) + }) + } +} + +func TestAPIError_Unwrap(t *testing.T) { + tests := []struct { + name string + apiErr *APIError + wantErr error + wantNil bool + }{ + { + name: "404 unwraps to ErrNotFound", + apiErr: &APIError{StatusCode: 404, Message: "Not Found"}, + wantErr: forge.ErrNotFound, + }, + { + name: "422 reference already exists unwraps to ErrAlreadyExists", + apiErr: &APIError{StatusCode: 422, Message: "Reference already exists"}, + wantErr: forge.ErrAlreadyExists, + }, + { + name: "422 PR already exists unwraps to ErrAlreadyExists", + apiErr: &APIError{ + StatusCode: 422, + Message: "Validation Failed", + Errors: []APIErrorDetail{ + {Resource: "PullRequest", Code: "custom", Message: "A pull request already exists for user:branch."}, + }, + }, + wantErr: forge.ErrAlreadyExists, + }, + { + name: "422 non-fast-forward does not unwrap", + apiErr: &APIError{StatusCode: 422, Message: "Update is not a fast forward"}, + wantNil: true, + }, + { + name: "403 does not unwrap", + apiErr: &APIError{StatusCode: 403, Message: "Resource not accessible by integration"}, + wantNil: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.apiErr.Unwrap() + if tt.wantNil { + assert.Nil(t, got) + } else { + assert.ErrorIs(t, got, tt.wantErr) + } + }) + } +} + func TestSecondaryRateLimit_RetriedWithoutRetryAfterHeader(t *testing.T) { attempts := 0 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { From b4c2edb879211d813387b07b37ccc5d42a812349 Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Mon, 15 Jun 2026 17:00:23 -0400 Subject: [PATCH 5/5] fix(forge): handle ruleset violations and improve scaffold resilience - Add "rule violation" to isBranchProtectionError for GitHub rulesets - Always attempt PR creation in CommitScaffoldFiles for crash recovery - Add comment explaining intentional Unwrap asymmetry for ErrBranchProtected - Correct ADR-0005 sentinel error wrapping description - Add ruleset violation test case and scaffold-branch-also-protected tests - Fix BranchUpToDate tests to inject ErrAlreadyExists for re-run scenario Assisted-by: Claude Signed-off-by: Wayne Sun --- docs/ADRs/0005-forge-abstraction-layer.md | 2 +- internal/cli/admin_test.go | 20 +++++++++++++- internal/forge/fake.go | 5 +++- internal/forge/github/github.go | 13 ++++++++- internal/forge/github/github_test.go | 11 ++++++++ internal/layers/commit.go | 32 ++++++++++++++--------- internal/layers/workflows_test.go | 18 +++++++++++-- 7 files changed, 83 insertions(+), 18 deletions(-) diff --git a/docs/ADRs/0005-forge-abstraction-layer.md b/docs/ADRs/0005-forge-abstraction-layer.md index 39da8c5b6..057976af5 100644 --- a/docs/ADRs/0005-forge-abstraction-layer.md +++ b/docs/ADRs/0005-forge-abstraction-layer.md @@ -34,5 +34,5 @@ No code outside `internal/forge/` imports forge-specific packages directly. - Forge-neutral naming occasionally feels awkward (e.g., `ChangeProposal`), but prevents GitHub-centric thinking from leaking into the model. - The interface will grow as new operations are needed; keeping it cohesive requires discipline. - The `FakeClient` enables deterministic testing of every layer without network calls. -- Sentinel errors (`ErrNotFound`, `ErrBranchProtected`, `ErrAlreadyExists`) with `errors.Is()` helpers provide forge-agnostic error classification. Forge implementations wrap these sentinels via `Unwrap()` so callers never inspect HTTP status codes or provider-specific messages directly. +- Sentinel errors (`ErrNotFound`, `ErrBranchProtected`, `ErrAlreadyExists`) with `errors.Is()` helpers provide forge-agnostic error classification. `ErrNotFound` and `ErrAlreadyExists` are mapped in `APIError.Unwrap()` for automatic propagation. `ErrBranchProtected` is wrapped contextually at the call site (e.g., `commitFilesTo`) where the operation context disambiguates branch-protection 422s from other validation failures. - `CommitFilesToBranch` complements `CommitFiles` (default branch) by targeting a specific branch, enabling the protected-branch fallback path where scaffold files are committed to a feature branch and delivered via PR. diff --git a/internal/cli/admin_test.go b/internal/cli/admin_test.go index a88f67970..797dfb485 100644 --- a/internal/cli/admin_test.go +++ b/internal/cli/admin_test.go @@ -2156,6 +2156,24 @@ func TestApplyPerRepoScaffold_ProtectedBranch_CommitToBranchFails(t *testing.T) assert.Contains(t, err.Error(), "committing scaffold files to branch") } +func TestApplyPerRepoScaffold_ProtectedBranch_ScaffoldBranchAlsoProtected(t *testing.T) { + client := forge.NewFakeClient() + client.Repos = []forge.Repository{{FullName: "acme/widget", DefaultBranch: "main"}} + client.Errors["CommitFiles"] = fmt.Errorf("%w: github api: 422", forge.ErrBranchProtected) + client.Errors["CommitFilesToBranch"] = fmt.Errorf("%w: scaffold branch also protected", forge.ErrBranchProtected) + printer := ui.New(&bytes.Buffer{}) + + files := []forge.TreeFile{ + {Path: ".fullsend/config.yaml", Content: []byte("cfg"), Mode: "100644"}, + } + + err := applyPerRepoScaffold(context.Background(), client, printer, + "acme", "widget", files, nil, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "scaffold branch") + assert.Contains(t, err.Error(), "configure branch protection") +} + func TestApplyPerRepoScaffold_ProtectedBranch_CreatePRFails(t *testing.T) { client := forge.NewFakeClient() client.Repos = []forge.Repository{{FullName: "acme/widget", DefaultBranch: "main"}} @@ -2198,6 +2216,7 @@ func TestApplyPerRepoScaffold_ProtectedBranch_BranchUpToDate(t *testing.T) { client := forge.NewFakeClient() client.Repos = []forge.Repository{{FullName: "acme/widget", DefaultBranch: "main"}} client.Errors["CommitFiles"] = fmt.Errorf("%w: github api: 422", forge.ErrBranchProtected) + client.Errors["CreateChangeProposal"] = fmt.Errorf("PR: %w", forge.ErrAlreadyExists) noChange := false client.CommitFilesChanged = &noChange var buf bytes.Buffer @@ -2211,6 +2230,5 @@ func TestApplyPerRepoScaffold_ProtectedBranch_BranchUpToDate(t *testing.T) { "acme", "widget", files, nil, nil) require.NoError(t, err) - assert.Empty(t, client.CreatedProposals, "should not create PR when branch files are unchanged") assert.Contains(t, buf.String(), "up to date") } diff --git a/internal/forge/fake.go b/internal/forge/fake.go index b0524a1b1..2b9863277 100644 --- a/internal/forge/fake.go +++ b/internal/forge/fake.go @@ -145,7 +145,10 @@ type FakeClient struct { IssueComments map[string][]IssueComment // key: "owner/repo/number" OpenIssues map[string][]Issue // key: "owner/repo" - // CommitFilesChanged controls the return value of CommitFiles (default true). + // CommitFilesChanged controls the return value of both CommitFiles and + // CommitFilesToBranch (default true). A single field suffices because + // callers that test the fallback path inject an error on CommitFiles, + // so only CommitFilesToBranch reads this value in practice. CommitFilesChanged *bool // Pull request head SHA for GetPullRequestHeadSHA. diff --git a/internal/forge/github/github.go b/internal/forge/github/github.go index 8fe147e73..b110b55c3 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -72,6 +72,13 @@ func (e *APIError) Error() string { } // Unwrap returns sentinel errors for well-known API responses. +// +// ErrBranchProtected is intentionally NOT mapped here. Branch protection +// 422s are context-dependent: the word "protected" in a validation error +// only signals a branch-protection failure when it comes from a ref update +// (PATCH /git/refs). Other 422s may coincidentally mention "protected" in +// unrelated contexts. The wrapping happens in commitFilesTo where the +// operation context is known. func (e *APIError) Unwrap() error { if e.StatusCode == http.StatusNotFound { return forge.ErrNotFound @@ -777,12 +784,16 @@ func (c *LiveClient) commitFilesTo(ctx context.Context, owner, repo, branch, mes // isBranchProtectionError checks whether a 422 APIError indicates branch // protection rather than another validation failure (e.g. non-fast-forward). +// It matches both legacy branch protection rules and newer repository rulesets. func isBranchProtectionError(apiErr *APIError) bool { msg := strings.ToLower(apiErr.Message) for _, d := range apiErr.Errors { msg += " " + strings.ToLower(d.Message) } - return strings.Contains(msg, "protected") || strings.Contains(msg, "required status") || strings.Contains(msg, "required review") + return strings.Contains(msg, "protected") || + strings.Contains(msg, "required status") || + strings.Contains(msg, "required review") || + strings.Contains(msg, "rule violation") } func isAlreadyExistsError(apiErr *APIError) bool { diff --git a/internal/forge/github/github_test.go b/internal/forge/github/github_test.go index 214c23dba..242fb9b5a 100644 --- a/internal/forge/github/github_test.go +++ b/internal/forge/github/github_test.go @@ -684,6 +684,17 @@ func TestIsBranchProtectionError(t *testing.T) { apiErr: &APIError{StatusCode: 422, Message: "Reference already exists"}, want: false, }, + { + name: "repository ruleset violation", + apiErr: &APIError{ + StatusCode: 422, + Message: "Update is not a fast forward", + Errors: []APIErrorDetail{ + {Message: "Repository rule violations found for refs/heads/main."}, + }, + }, + want: true, + }, { name: "validation failed for unrelated reason", apiErr: &APIError{ diff --git a/internal/layers/commit.go b/internal/layers/commit.go index 1a3979834..63789d9c6 100644 --- a/internal/layers/commit.go +++ b/internal/layers/commit.go @@ -18,6 +18,11 @@ func CommitScaffoldFiles(ctx context.Context, client forge.Client, printer *ui.P if err != nil && forge.IsBranchProtected(err) { printer.StepWarn("Default branch is protected — creating scaffold PR instead") + // The branch name is fixed so that re-runs update the same PR rather + // than creating a new one each time. If the branch already exists from + // a prior run, we commit on top of it. The branch may be behind the + // current default branch, which can produce merge conflicts in the PR; + // this is acceptable because the user must merge the PR manually anyway. const scaffoldBranch = "fullsend/scaffold-install" if branchErr := client.CreateBranch(ctx, owner, repo, scaffoldBranch); branchErr != nil { if !forge.IsAlreadyExists(branchErr) { @@ -36,22 +41,25 @@ func CommitScaffoldFiles(ctx context.Context, client forge.Client, printer *ui.P return fmt.Errorf("committing scaffold files to branch: %w", commitErr) } - if branchCommitted { - proposal, prErr := client.CreateChangeProposal(ctx, owner, repo, - prTitle, prBody, scaffoldBranch, defaultBranch) - if prErr != nil { - if !forge.IsAlreadyExists(prErr) { - printer.StepFail("Failed to create scaffold PR") - return fmt.Errorf("creating scaffold PR: %w", prErr) - } - printer.StepDone("Scaffold PR already exists") + // Always attempt PR creation — even when branchCommitted is false. + // A prior run may have committed to the branch but crashed before + // creating the PR. ErrAlreadyExists handles the common re-run case. + proposal, prErr := client.CreateChangeProposal(ctx, owner, repo, + prTitle, prBody, scaffoldBranch, defaultBranch) + if prErr != nil { + if !forge.IsAlreadyExists(prErr) { + printer.StepFail("Failed to create scaffold PR") + return fmt.Errorf("creating scaffold PR: %w", prErr) + } + if branchCommitted { + printer.StepDone("Scaffold PR already exists — updated with new files") } else { - printer.StepDone(fmt.Sprintf("Created PR #%d: %s", proposal.Number, proposal.URL)) + printer.StepDone("Scaffold branch and PR up to date") } - printer.StepInfo("Merge the PR to activate fullsend workflows") } else { - printer.StepDone("Scaffold branch up to date") + printer.StepDone(fmt.Sprintf("Created PR #%d: %s", proposal.Number, proposal.URL)) } + printer.StepInfo("Merge the PR to activate fullsend workflows") } else if err != nil { printer.StepFail("Failed to commit scaffold files") return fmt.Errorf("committing scaffold files: %w", err) diff --git a/internal/layers/workflows_test.go b/internal/layers/workflows_test.go index bbec4c977..6030845c1 100644 --- a/internal/layers/workflows_test.go +++ b/internal/layers/workflows_test.go @@ -189,6 +189,19 @@ func TestWorkflowsLayer_Install_ProtectedBranch_CommitToBranchFails(t *testing.T assert.Contains(t, err.Error(), "committing scaffold files to branch") } +func TestWorkflowsLayer_Install_ProtectedBranch_ScaffoldBranchAlsoProtected(t *testing.T) { + client := forge.NewFakeClient() + client.Repos = []forge.Repository{{FullName: "test-org/.fullsend", DefaultBranch: "main"}} + client.Errors["CommitFiles"] = fmt.Errorf("%w: github api: 422", forge.ErrBranchProtected) + client.Errors["CommitFilesToBranch"] = fmt.Errorf("%w: scaffold branch also protected", forge.ErrBranchProtected) + layer, _ := newWorkflowsLayer(t, client) + + err := layer.Install(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "scaffold branch") + assert.Contains(t, err.Error(), "configure branch protection") +} + func TestWorkflowsLayer_Install_ProtectedBranch_CreatePRFails(t *testing.T) { client := forge.NewFakeClient() client.Repos = []forge.Repository{{FullName: "test-org/.fullsend", DefaultBranch: "main"}} @@ -220,6 +233,7 @@ func TestWorkflowsLayer_Install_ProtectedBranch_BranchUpToDate(t *testing.T) { client := forge.NewFakeClient() client.Repos = []forge.Repository{{FullName: "test-org/.fullsend", DefaultBranch: "main"}} client.Errors["CommitFiles"] = fmt.Errorf("%w: github api: 422", forge.ErrBranchProtected) + client.Errors["CreateChangeProposal"] = fmt.Errorf("PR: %w", forge.ErrAlreadyExists) noChange := false client.CommitFilesChanged = &noChange layer, buf := newWorkflowsLayer(t, client) @@ -227,8 +241,8 @@ func TestWorkflowsLayer_Install_ProtectedBranch_BranchUpToDate(t *testing.T) { err := layer.Install(context.Background()) require.NoError(t, err) - assert.Empty(t, client.CreatedProposals, "should not create PR when branch files are unchanged") - assert.Contains(t, buf.String(), "up to date") + output := buf.String() + assert.Contains(t, output, "up to date") } func TestWorkflowsLayer_Install_Error(t *testing.T) {