From 386a6d6ff69d4ac1115610494a2b835bc92d95ee Mon Sep 17 00:00:00 2001 From: fullsend-code <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Wed, 24 Jun 2026 19:03:02 +0000 Subject: [PATCH 1/3] feat(cli): add --pr and --direct flags to sync-scaffold, enroll, and unenroll MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sync-scaffold, enroll, and unenroll previously always pushed directly to the default branch, bypassing branch protection and review. This change adds --pr and --direct flags to all three commands for explicit delivery control: - sync-scaffold defaults to PR delivery (matching setup), with --direct to opt into direct push. This command changes 29+ files, so review-by- default is appropriate. - enroll/unenroll default to direct push, with --pr to opt into PR delivery. These toggle a single config value, so the overhead of a PR is not warranted by default. - --pr and --direct are mutually exclusive on all commands. When --pr is used for enroll/unenroll, config.yaml is committed to the fullsend/scaffold-install branch and delivered via PR using the same CommitScaffoldFiles helper that setup uses. The repo-maintenance workflow is not dispatched in PR mode — it runs automatically when the PR is merged. Closes #2625 --- internal/cli/admin.go | 70 +++++++++++++++++++++--- internal/cli/admin_test.go | 104 +++++++++++++++++++++++++++--------- internal/cli/github.go | 23 ++++++-- internal/cli/github_test.go | 72 +++++++++++++++++++++++-- 4 files changed, 230 insertions(+), 39 deletions(-) diff --git a/internal/cli/admin.go b/internal/cli/admin.go index 2d3add2a70..e0c7ca7ccf 100644 --- a/internal/cli/admin.go +++ b/internal/cli/admin.go @@ -2144,13 +2144,18 @@ func newDisableCmd() *cobra.Command { } // reposRunFunc is the signature for repo enable/disable operations. -type reposRunFunc func(ctx context.Context, client forge.Client, printer *ui.Printer, org string, repos []string, all bool, yolo bool) error +type reposRunFunc func(ctx context.Context, client forge.Client, printer *ui.Printer, org string, repos []string, all bool, yolo bool, pr bool) error // newReposSubcommand creates a repos enable or disable subcommand with shared setup logic. // If withYolo is true, the --yolo flag is added to skip confirmation prompts. +// defaultDirect controls the delivery default: when true (the default for +// enroll/unenroll), changes are pushed directly and --pr opts into PR delivery; +// when false, changes go via PR and --direct opts into direct push. func newReposSubcommand(use, short, long, allFlagHelp string, runFn reposRunFunc, withYolo bool) *cobra.Command { var all bool var yolo bool + var prFlag bool + var directFlag bool cmd := &cobra.Command{ Use: use, @@ -2158,6 +2163,10 @@ func newReposSubcommand(use, short, long, allFlagHelp string, runFn reposRunFunc Long: long, Args: cobra.MinimumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + if prFlag && directFlag { + return fmt.Errorf("--pr and --direct are mutually exclusive") + } + org := args[0] if err := validateOrgName(org); err != nil { return err @@ -2186,11 +2195,18 @@ func newReposSubcommand(use, short, long, allFlagHelp string, runFn reposRunFunc printer := ui.New(os.Stdout) ctx := cmd.Context() - return runFn(ctx, client, printer, org, repos, all, yolo) + // Default is direct push; --pr overrides to PR delivery. + // --direct is accepted for symmetry but is a no-op since it + // matches the default. + usePR := prFlag + + return runFn(ctx, client, printer, org, repos, all, yolo, usePR) }, } cmd.Flags().BoolVar(&all, "all", false, allFlagHelp) + cmd.Flags().BoolVar(&prFlag, "pr", false, "deliver changes via a pull request instead of pushing directly") + cmd.Flags().BoolVar(&directFlag, "direct", false, "push changes directly to the default branch (default behavior)") if withYolo { cmd.Flags().BoolVar(&yolo, "yolo", false, "skip confirmation prompt") } @@ -2223,7 +2239,7 @@ func newDisableReposCmd() *cobra.Command { // runEnableRepos enables the specified repositories for fullsend enrollment. // The yolo parameter is accepted for signature compatibility with reposRunFunc but is unused // since enable has no destructive operations that require confirmation. -func runEnableRepos(ctx context.Context, client forge.Client, printer *ui.Printer, org string, repos []string, all bool, yolo bool) error { +func runEnableRepos(ctx context.Context, client forge.Client, printer *ui.Printer, org string, repos []string, all bool, yolo bool, pr bool) error { printer.Banner(Version()) printer.Blank() printer.Header("Enabling repositories for " + org) @@ -2326,7 +2342,7 @@ func runEnableRepos(ctx context.Context, client forge.Client, printer *ui.Printe // Save updated config. commitMsg := fmt.Sprintf("chore: enable %d repositories for fullsend enrollment", changed) var err error - dispatchTime, err = saveRepoConfig(ctx, client, printer, org, cfg, commitMsg) + dispatchTime, err = saveRepoConfig(ctx, client, printer, org, cfg, commitMsg, pr) if err != nil { return err } @@ -2403,7 +2419,7 @@ func syncOrgVariableVisibility(ctx context.Context, client forge.Client, printer } // runDisableRepos disables the specified repositories from fullsend enrollment. -func runDisableRepos(ctx context.Context, client forge.Client, printer *ui.Printer, org string, repos []string, all bool, yolo bool) error { +func runDisableRepos(ctx context.Context, client forge.Client, printer *ui.Printer, org string, repos []string, all bool, yolo bool, pr bool) error { printer.Banner(Version()) printer.Blank() printer.Header("Disabling repositories for " + org) @@ -2491,7 +2507,7 @@ func runDisableRepos(ctx context.Context, client forge.Client, printer *ui.Print // Save updated config. commitMsg := fmt.Sprintf("chore: disable %d repositories from fullsend enrollment", changed) - dispatchTime, err := saveRepoConfig(ctx, client, printer, org, cfg, commitMsg) + dispatchTime, err := saveRepoConfig(ctx, client, printer, org, cfg, commitMsg, pr) if err != nil { return err } @@ -2565,7 +2581,11 @@ func loadRepoConfig(ctx context.Context, client forge.Client, printer *ui.Printe // saveRepoConfig marshals the config, commits it, and dispatches the // repo-maintenance workflow. It returns the dispatch time so callers can // watch the resulting workflow run. A zero time means the dispatch failed. -func saveRepoConfig(ctx context.Context, client forge.Client, printer *ui.Printer, org string, cfg *config.OrgConfig, commitMsg string) (time.Time, error) { +// +// When pr is true, config.yaml is delivered via a pull request instead of +// being pushed directly to the default branch. The repo-maintenance +// workflow is not dispatched in PR mode — it will run when the PR is merged. +func saveRepoConfig(ctx context.Context, client forge.Client, printer *ui.Printer, org string, cfg *config.OrgConfig, commitMsg string, pr bool) (time.Time, error) { // Marshal updated config. updatedConfigData, err := cfg.Marshal() if err != nil { @@ -2573,6 +2593,10 @@ func saveRepoConfig(ctx context.Context, client forge.Client, printer *ui.Printe return time.Time{}, fmt.Errorf("marshaling config.yaml: %w", err) } + if pr { + return saveRepoConfigViaPR(ctx, client, printer, org, updatedConfigData, commitMsg) + } + // Commit and push changes. printer.StepStart("Committing changes to .fullsend") if err := client.CreateOrUpdateFile(ctx, org, forge.ConfigRepoName, "config.yaml", commitMsg, updatedConfigData); err != nil { @@ -2596,6 +2620,38 @@ func saveRepoConfig(ctx context.Context, client forge.Client, printer *ui.Printe return dispatchTime, nil } +// saveRepoConfigViaPR delivers config.yaml via a pull request. Uses the +// same branch/PR pattern as scaffold PR delivery: a fixed branch name so +// re-runs update the same PR rather than creating a new one. +func saveRepoConfigViaPR(ctx context.Context, client forge.Client, printer *ui.Printer, org string, configData []byte, commitMsg string) (time.Time, error) { + cfgRepo, err := client.GetRepo(ctx, org, forge.ConfigRepoName) + if err != nil { + printer.StepFail("Failed to get .fullsend repo info") + return time.Time{}, fmt.Errorf("getting config repo info: %w", err) + } + + files := []forge.TreeFile{{ + Path: "config.yaml", + Content: configData, + Mode: "100644", + }} + + prTitle := "chore: update fullsend enrollment config" + prBody := "This PR updates `config.yaml` in the .fullsend config repo.\n\n" + + "Merge this PR to apply the enrollment changes. The repo-maintenance workflow will run automatically on merge." + + printer.StepStart("Creating enrollment config PR") + _, prErr := layers.CommitScaffoldFiles(ctx, client, printer, + org, forge.ConfigRepoName, cfgRepo.DefaultBranch, + commitMsg, prTitle, prBody, files, false) + if prErr != nil { + return time.Time{}, prErr + } + + // No workflow dispatch in PR mode — repo-maintenance runs on merge. + return time.Time{}, nil +} + // awaitRepoMaintenance watches the repo-maintenance workflow run triggered by a // config.yaml push, waits for it to complete, and prints any PR URLs from its // annotations. diff --git a/internal/cli/admin_test.go b/internal/cli/admin_test.go index 83b69b1792..b36ebe9010 100644 --- a/internal/cli/admin_test.go +++ b/internal/cli/admin_test.go @@ -539,7 +539,7 @@ func TestReposEnableCmd_AllIgnoresPositionalArgs(t *testing.T) { printer := ui.New(&discardWriter{}) // Pass "web-app" as a positional arg, but --all should ignore it and enable both repos - err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, true, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, true, true, false) require.NoError(t, err) // Verify both repos were enabled (--all behavior), not just web-app @@ -560,7 +560,7 @@ func TestReposDisableCmd_AllIgnoresPositionalArgs(t *testing.T) { printer := ui.New(&discardWriter{}) // Pass "web-app" as a positional arg, but --all should ignore it and disable both repos - err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, true, true) + err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, true, true, false) require.NoError(t, err) // Verify both repos were disabled (--all behavior), not just web-app @@ -619,7 +619,7 @@ func TestRunEnableRepos_EnableSingleRepo(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app", "api"}) printer := ui.New(&discardWriter{}) - err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true, false) require.NoError(t, err) // Verify config was updated. @@ -639,7 +639,7 @@ func TestRunEnableRepos_EnableMultipleRepos(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app", "api", "docs"}) printer := ui.New(&discardWriter{}) - err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app", "docs"}, false, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app", "docs"}, false, true, false) require.NoError(t, err) // Verify config was updated. @@ -659,7 +659,7 @@ func TestRunEnableRepos_EnableAllRepos(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app", "api", "new-repo"}) printer := ui.New(&discardWriter{}) - err := runEnableRepos(context.Background(), client, printer, "testorg", nil, true, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", nil, true, true, false) require.NoError(t, err) // Verify all repos were enabled (excluding .fullsend). @@ -681,7 +681,7 @@ func TestRunEnableRepos_NoOpWhenAlreadyEnabled(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app"}) printer := ui.New(&discardWriter{}) - err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true, false) require.NoError(t, err) // Verify no file was created (no changes). @@ -692,7 +692,7 @@ func TestRunEnableRepos_ErrorWhenFullsendRepoMissing(t *testing.T) { client := forge.NewFakeClient() printer := ui.New(&discardWriter{}) - err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true, false) require.Error(t, err) assert.Contains(t, err.Error(), ".fullsend repository not found") } @@ -701,7 +701,7 @@ func TestRunEnableRepos_ErrorWhenConfigMissing(t *testing.T) { client := setupTestClient("testorg", nil, []string{"web-app"}) printer := ui.New(&discardWriter{}) - err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true, false) require.Error(t, err) assert.Contains(t, err.Error(), "reading config.yaml") } @@ -713,7 +713,7 @@ func TestRunEnableRepos_ErrorWhenEnablingFullsend(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app"}) printer := ui.New(&discardWriter{}) - err := runEnableRepos(context.Background(), client, printer, "testorg", []string{".fullsend"}, false, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{".fullsend"}, false, true, false) require.Error(t, err) assert.Contains(t, err.Error(), "cannot enable .fullsend repository") } @@ -725,7 +725,7 @@ func TestRunEnableRepos_ErrorWhenRepoNotFound(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app"}) printer := ui.New(&discardWriter{}) - err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"nonexistent"}, false, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"nonexistent"}, false, true, false) require.Error(t, err) assert.Contains(t, err.Error(), "repository nonexistent not found") } @@ -738,7 +738,7 @@ func TestRunEnableRepos_CommitMessageFormat(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app", "api"}) printer := ui.New(&discardWriter{}) - err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app", "api"}, false, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app", "api"}, false, true, false) require.NoError(t, err) require.Len(t, client.CreatedFiles, 1) @@ -773,7 +773,7 @@ func TestRunEnableRepos_UpdatesOrgVariableVisibility(t *testing.T) { printer := ui.New(&discardWriter{}) // Action: enable repo "api". - err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"api"}, false, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"api"}, false, true, false) require.NoError(t, err) // Assert: SetOrgVariableRepos was called with both enrolled repo IDs @@ -797,7 +797,7 @@ func TestRunEnableRepos_SkipsVariableSyncWhenNotOIDCMint(t *testing.T) { printer := ui.New(&discardWriter{}) - err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true, false) require.NoError(t, err) // SetOrgVariableRepos should not have been called. @@ -826,7 +826,7 @@ func TestRunEnableRepos_VariableSyncErrorDoesNotBlockEnable(t *testing.T) { printer := ui.New(&discardWriter{}) - err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true, false) require.NoError(t, err, "enable should succeed even when variable sync fails") } @@ -843,7 +843,7 @@ func TestRunEnableRepos_SkipsVariableSyncWhenVariableNotExists(t *testing.T) { printer := ui.New(&discardWriter{}) - err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true, false) require.NoError(t, err) // SetOrgVariableRepos should not have been called. @@ -860,7 +860,7 @@ func TestRunDisableRepos_DisableSingleRepo(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app", "api"}) printer := ui.New(&discardWriter{}) - err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true) + err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true, false) require.NoError(t, err) // Verify config was updated. @@ -880,7 +880,7 @@ func TestRunDisableRepos_DisableMultipleRepos(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app", "api", "docs"}) printer := ui.New(&discardWriter{}) - err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app", "docs"}, false, true) + err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app", "docs"}, false, true, false) require.NoError(t, err) // Verify config was updated. @@ -900,7 +900,7 @@ func TestRunDisableRepos_DisableAllRepos(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app", "api"}) printer := ui.New(&discardWriter{}) - err := runDisableRepos(context.Background(), client, printer, "testorg", nil, true, true) + err := runDisableRepos(context.Background(), client, printer, "testorg", nil, true, true, false) require.NoError(t, err) // Verify all repos were disabled. @@ -918,7 +918,7 @@ func TestRunDisableRepos_NoOpWhenAlreadyDisabled(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app"}) printer := ui.New(&discardWriter{}) - err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true) + err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true, false) require.NoError(t, err) // Verify no file was created (no changes). @@ -929,7 +929,7 @@ func TestRunDisableRepos_ErrorWhenFullsendRepoMissing(t *testing.T) { client := forge.NewFakeClient() printer := ui.New(&discardWriter{}) - err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true) + err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true, false) require.Error(t, err) assert.Contains(t, err.Error(), ".fullsend repository not found") } @@ -938,7 +938,7 @@ func TestRunDisableRepos_ErrorWhenConfigMissing(t *testing.T) { client := setupTestClient("testorg", nil, []string{"web-app"}) printer := ui.New(&discardWriter{}) - err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true) + err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true, false) require.Error(t, err) assert.Contains(t, err.Error(), "reading config.yaml") } @@ -950,7 +950,7 @@ func TestRunDisableRepos_ErrorWhenDisablingFullsend(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app"}) printer := ui.New(&discardWriter{}) - err := runDisableRepos(context.Background(), client, printer, "testorg", []string{".fullsend"}, false, true) + err := runDisableRepos(context.Background(), client, printer, "testorg", []string{".fullsend"}, false, true, false) require.Error(t, err) assert.Contains(t, err.Error(), "cannot disable .fullsend repository") } @@ -963,7 +963,7 @@ func TestRunDisableRepos_AllowsRepoNotInConfig(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app"}) printer := ui.New(&discardWriter{}) - err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"nonexistent"}, false, true) + err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"nonexistent"}, false, true, false) require.NoError(t, err) // Should succeed but make no changes (repo not in config, nothing to disable) assert.Len(t, client.CreatedFiles, 0) @@ -977,13 +977,69 @@ func TestRunDisableRepos_CommitMessageFormat(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app", "api"}) printer := ui.New(&discardWriter{}) - err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app", "api"}, false, true) + err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app", "api"}, false, true, false) require.NoError(t, err) require.Len(t, client.CreatedFiles, 1) assert.Contains(t, client.CreatedFiles[0].Message, "chore: disable 2 repositories") } +func TestRunEnableRepos_PRDelivery(t *testing.T) { + cfg := setupTestConfig(map[string]bool{ + "web-app": false, + "api": false, + }) + client := setupTestClient("testorg", cfg, []string{"web-app", "api"}) + printer := ui.New(&discardWriter{}) + + // pr=true should create a branch and PR instead of pushing directly. + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true, true) + require.NoError(t, err) + + // PR mode: should create a branch and proposal, not use CreateOrUpdateFile. + assert.NotEmpty(t, client.CreatedBranches, "expected a branch to be created for PR delivery") + assert.NotEmpty(t, client.CreatedProposals, "expected a PR to be created") + // CreatedFiles records CreateOrUpdateFile calls (direct commits). + // In PR mode these should be empty. + assert.Empty(t, client.CreatedFiles, "expected no direct file commits in PR mode") +} + +func TestRunDisableRepos_PRDelivery(t *testing.T) { + cfg := setupTestConfig(map[string]bool{ + "web-app": true, + "api": true, + }) + client := setupTestClient("testorg", cfg, []string{"web-app", "api"}) + printer := ui.New(&discardWriter{}) + + // pr=true should create a branch and PR instead of pushing directly. + err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true, true) + require.NoError(t, err) + + // PR mode: should create a branch and proposal, not use CreateOrUpdateFile. + assert.NotEmpty(t, client.CreatedBranches, "expected a branch to be created for PR delivery") + assert.NotEmpty(t, client.CreatedProposals, "expected a PR to be created") + assert.Empty(t, client.CreatedFiles, "expected no direct file commits in PR mode") +} + +func TestReposEnableCmd_HasDeliveryFlags(t *testing.T) { + cmd := newEnableReposCmd() + prFlag := cmd.Flags().Lookup("pr") + require.NotNil(t, prFlag, "expected --pr flag") + assert.Equal(t, "false", prFlag.DefValue) + directFlag := cmd.Flags().Lookup("direct") + require.NotNil(t, directFlag, "expected --direct flag") + assert.Equal(t, "false", directFlag.DefValue) +} + +func TestReposDisableCmd_HasDeliveryFlags(t *testing.T) { + cmd := newDisableReposCmd() + prFlag := cmd.Flags().Lookup("pr") + require.NotNil(t, prFlag, "expected --pr flag") + directFlag := cmd.Flags().Lookup("direct") + require.NotNil(t, directFlag, "expected --direct flag") +} + func TestPromptEnrollment_ChooseAll(t *testing.T) { tests := []struct { name string diff --git a/internal/cli/github.go b/internal/cli/github.go index b4d4e0eaa4..36f4c5e89a 100644 --- a/internal/cli/github.go +++ b/internal/cli/github.go @@ -929,12 +929,19 @@ func runGitHubUninstall(ctx context.Context, client forge.Client, printer *ui.Pr // --- sync-scaffold command --- func newGitHubSyncScaffoldCmd() *cobra.Command { + var directFlag bool + var prFlag bool + cmd := &cobra.Command{ Use: "sync-scaffold ", Short: "Update workflow templates in .fullsend", - Long: "Re-commits scaffold files (shim and maintenance workflows) to the .fullsend repo without touching secrets, variables, or enrollment. Useful after fullsend version upgrades. Idempotent and safe to run repeatedly.", + Long: "Re-commits scaffold files (shim and maintenance workflows) to the .fullsend repo without touching secrets, variables, or enrollment. Useful after fullsend version upgrades. Idempotent and safe to run repeatedly.\n\nBy default, changes are delivered via a pull request. Use --direct to push to the default branch instead.", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + if prFlag && directFlag { + return fmt.Errorf("--pr and --direct are mutually exclusive") + } + org := args[0] if err := validateOrgName(org); err != nil { return err @@ -948,15 +955,23 @@ func newGitHubSyncScaffoldCmd() *cobra.Command { client := gh.New(token) printer := ui.New(os.Stdout) - return runGitHubSyncScaffold(cmd.Context(), client, printer, org) + // Default is PR delivery; --direct overrides to direct push. + // --pr is accepted for symmetry but is a no-op since it + // matches the default. + direct := directFlag + + return runGitHubSyncScaffold(cmd.Context(), client, printer, org, direct) }, } + cmd.Flags().BoolVar(&directFlag, "direct", false, "push scaffold files directly to the default branch instead of creating a PR") + cmd.Flags().BoolVar(&prFlag, "pr", false, "deliver changes via a pull request (default behavior)") + return cmd } // runGitHubSyncScaffold runs only the WorkflowsLayer. -func runGitHubSyncScaffold(ctx context.Context, client forge.Client, printer *ui.Printer, org string) error { +func runGitHubSyncScaffold(ctx context.Context, client forge.Client, printer *ui.Printer, org string, direct bool) error { printer.Banner(Version()) printer.Blank() printer.Header("Syncing scaffold for " + org) @@ -983,7 +998,7 @@ func runGitHubSyncScaffold(ctx context.Context, client forge.Client, printer *ui } upstreamRef, upstreamTag := resolveUpstreamRef() - wfLayer := layers.NewWorkflowsLayer(org, client, printer, user, version, vendored).WithDirect(true).WithUpstreamRef(upstreamRef, upstreamTag) + wfLayer := layers.NewWorkflowsLayer(org, client, printer, user, version, vendored).WithDirect(direct).WithUpstreamRef(upstreamRef, upstreamTag) if id, idErr := client.GetAuthenticatedUserIdentity(ctx); idErr == nil { wfLayer = wfLayer.WithSignOff(id.Name, id.Email) } diff --git a/internal/cli/github_test.go b/internal/cli/github_test.go index 7f19e9362b..6bac824cbb 100644 --- a/internal/cli/github_test.go +++ b/internal/cli/github_test.go @@ -237,7 +237,7 @@ func TestGitHubEnrollCmd_DelegatesCorrectly(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app", "api"}) printer := ui.New(&discardWriter{}) - err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true, false) require.NoError(t, err) require.Len(t, client.CreatedFiles, 1) @@ -544,7 +544,7 @@ func TestRunGitHubSyncScaffold_CommitsFiles(t *testing.T) { client.AuthenticatedUser = "testuser" printer := ui.New(&discardWriter{}) - err := runGitHubSyncScaffold(context.Background(), client, printer, "acme") + err := runGitHubSyncScaffold(context.Background(), client, printer, "acme", true) require.NoError(t, err) // sync-scaffold uses direct mode — files are committed to the default branch. @@ -563,7 +563,7 @@ func TestRunGitHubSyncScaffold_VendoredMarker(t *testing.T) { } printer := ui.New(&discardWriter{}) - err := runGitHubSyncScaffold(context.Background(), client, printer, "acme") + err := runGitHubSyncScaffold(context.Background(), client, printer, "acme", true) require.NoError(t, err) require.NotEmpty(t, client.CommittedFiles) } @@ -577,11 +577,75 @@ func TestRunGitHubSyncScaffold_InvalidConfig(t *testing.T) { } printer := ui.New(&discardWriter{}) - err := runGitHubSyncScaffold(context.Background(), client, printer, "acme") + err := runGitHubSyncScaffold(context.Background(), client, printer, "acme", true) require.Error(t, err) assert.Contains(t, err.Error(), "parsing config.yaml") } +func TestRunGitHubSyncScaffold_DefaultCreatesPR(t *testing.T) { + client := forge.NewFakeClient() + client.Repos = []forge.Repository{ + {Name: ".fullsend", FullName: "acme/.fullsend"}, + } + client.AuthenticatedUser = "testuser" + printer := ui.New(&discardWriter{}) + + // direct=false means PR-based delivery (the default). + err := runGitHubSyncScaffold(context.Background(), client, printer, "acme", false) + require.NoError(t, err) + + // Should create a branch and PR, not commit directly. + assert.NotEmpty(t, client.CreatedBranches, "expected a scaffold branch to be created") + assert.NotEmpty(t, client.CreatedProposals, "expected a scaffold PR to be created") + assert.Empty(t, client.CommittedFiles, "expected no direct commits when using PR delivery") +} + +func TestGitHubSyncScaffoldCmd_HasDeliveryFlags(t *testing.T) { + cmd := newGitHubSyncScaffoldCmd() + directFlag := cmd.Flags().Lookup("direct") + require.NotNil(t, directFlag, "expected --direct flag") + assert.Equal(t, "false", directFlag.DefValue) + prFlag := cmd.Flags().Lookup("pr") + require.NotNil(t, prFlag, "expected --pr flag") + assert.Equal(t, "false", prFlag.DefValue) +} + +func TestGitHubSyncScaffoldCmd_MutualExclusiveFlags(t *testing.T) { + t.Setenv("GH_TOKEN", "test-token") + cmd := newRootCmd() + cmd.SetArgs([]string{"github", "sync-scaffold", "--pr", "--direct", "acme"}) + err := cmd.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), "--pr and --direct are mutually exclusive") +} + +func TestGitHubEnrollCmd_HasDeliveryFlags(t *testing.T) { + cmd := newGitHubEnrollCmd() + prFlag := cmd.Flags().Lookup("pr") + require.NotNil(t, prFlag, "expected --pr flag") + assert.Equal(t, "false", prFlag.DefValue) + directFlag := cmd.Flags().Lookup("direct") + require.NotNil(t, directFlag, "expected --direct flag") + assert.Equal(t, "false", directFlag.DefValue) +} + +func TestGitHubUnenrollCmd_HasDeliveryFlags(t *testing.T) { + cmd := newGitHubUnenrollCmd() + prFlag := cmd.Flags().Lookup("pr") + require.NotNil(t, prFlag, "expected --pr flag") + directFlag := cmd.Flags().Lookup("direct") + require.NotNil(t, directFlag, "expected --direct flag") +} + +func TestGitHubEnrollCmd_MutualExclusiveFlags(t *testing.T) { + t.Setenv("GH_TOKEN", "test-token") + cmd := newRootCmd() + cmd.SetArgs([]string{"github", "enroll", "--pr", "--direct", "--all", "acme"}) + err := cmd.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), "--pr and --direct are mutually exclusive") +} + func TestRunGitHubSetupPerOrg_DryRun(t *testing.T) { client := forge.NewFakeClient() client.AuthenticatedUser = "testuser" From 71c97d8ab57e0c6203795711f5662a6001682986 Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Fri, 26 Jun 2026 14:12:09 +0000 Subject: [PATCH 2/3] fix(cli): make PR delivery the default for enroll/unenroll, remove --pr flag Remove the --pr flag from sync-scaffold, enroll, and unenroll commands. PR delivery is now the default for all three commands. Users who need direct push can use --direct to opt out. Addresses review feedback on #2630 --- internal/cli/admin.go | 19 +++++-------------- internal/cli/admin_test.go | 13 ++++++------- internal/cli/github.go | 12 +----------- internal/cli/github_test.go | 38 +++++++++---------------------------- 4 files changed, 21 insertions(+), 61 deletions(-) diff --git a/internal/cli/admin.go b/internal/cli/admin.go index e0c7ca7ccf..235b2acade 100644 --- a/internal/cli/admin.go +++ b/internal/cli/admin.go @@ -2148,13 +2148,11 @@ type reposRunFunc func(ctx context.Context, client forge.Client, printer *ui.Pri // newReposSubcommand creates a repos enable or disable subcommand with shared setup logic. // If withYolo is true, the --yolo flag is added to skip confirmation prompts. -// defaultDirect controls the delivery default: when true (the default for -// enroll/unenroll), changes are pushed directly and --pr opts into PR delivery; -// when false, changes go via PR and --direct opts into direct push. +// By default, changes are delivered via a pull request. Use --direct to push +// changes directly to the default branch instead. func newReposSubcommand(use, short, long, allFlagHelp string, runFn reposRunFunc, withYolo bool) *cobra.Command { var all bool var yolo bool - var prFlag bool var directFlag bool cmd := &cobra.Command{ @@ -2163,10 +2161,6 @@ func newReposSubcommand(use, short, long, allFlagHelp string, runFn reposRunFunc Long: long, Args: cobra.MinimumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - if prFlag && directFlag { - return fmt.Errorf("--pr and --direct are mutually exclusive") - } - org := args[0] if err := validateOrgName(org); err != nil { return err @@ -2195,18 +2189,15 @@ func newReposSubcommand(use, short, long, allFlagHelp string, runFn reposRunFunc printer := ui.New(os.Stdout) ctx := cmd.Context() - // Default is direct push; --pr overrides to PR delivery. - // --direct is accepted for symmetry but is a no-op since it - // matches the default. - usePR := prFlag + // Default is PR delivery; --direct overrides to direct push. + usePR := !directFlag return runFn(ctx, client, printer, org, repos, all, yolo, usePR) }, } cmd.Flags().BoolVar(&all, "all", false, allFlagHelp) - cmd.Flags().BoolVar(&prFlag, "pr", false, "deliver changes via a pull request instead of pushing directly") - cmd.Flags().BoolVar(&directFlag, "direct", false, "push changes directly to the default branch (default behavior)") + cmd.Flags().BoolVar(&directFlag, "direct", false, "push changes directly to the default branch instead of creating a PR") if withYolo { cmd.Flags().BoolVar(&yolo, "yolo", false, "skip confirmation prompt") } diff --git a/internal/cli/admin_test.go b/internal/cli/admin_test.go index b36ebe9010..94ad440d59 100644 --- a/internal/cli/admin_test.go +++ b/internal/cli/admin_test.go @@ -1022,22 +1022,21 @@ func TestRunDisableRepos_PRDelivery(t *testing.T) { assert.Empty(t, client.CreatedFiles, "expected no direct file commits in PR mode") } -func TestReposEnableCmd_HasDeliveryFlags(t *testing.T) { +func TestReposEnableCmd_HasDirectFlag(t *testing.T) { cmd := newEnableReposCmd() - prFlag := cmd.Flags().Lookup("pr") - require.NotNil(t, prFlag, "expected --pr flag") - assert.Equal(t, "false", prFlag.DefValue) directFlag := cmd.Flags().Lookup("direct") require.NotNil(t, directFlag, "expected --direct flag") assert.Equal(t, "false", directFlag.DefValue) + // --pr flag should not exist; PR delivery is the default. + assert.Nil(t, cmd.Flags().Lookup("pr"), "unexpected --pr flag; PR delivery is the default") } -func TestReposDisableCmd_HasDeliveryFlags(t *testing.T) { +func TestReposDisableCmd_HasDirectFlag(t *testing.T) { cmd := newDisableReposCmd() - prFlag := cmd.Flags().Lookup("pr") - require.NotNil(t, prFlag, "expected --pr flag") directFlag := cmd.Flags().Lookup("direct") require.NotNil(t, directFlag, "expected --direct flag") + // --pr flag should not exist; PR delivery is the default. + assert.Nil(t, cmd.Flags().Lookup("pr"), "unexpected --pr flag; PR delivery is the default") } func TestPromptEnrollment_ChooseAll(t *testing.T) { diff --git a/internal/cli/github.go b/internal/cli/github.go index 36f4c5e89a..b40bdb18ab 100644 --- a/internal/cli/github.go +++ b/internal/cli/github.go @@ -930,7 +930,6 @@ func runGitHubUninstall(ctx context.Context, client forge.Client, printer *ui.Pr func newGitHubSyncScaffoldCmd() *cobra.Command { var directFlag bool - var prFlag bool cmd := &cobra.Command{ Use: "sync-scaffold ", @@ -938,10 +937,6 @@ func newGitHubSyncScaffoldCmd() *cobra.Command { Long: "Re-commits scaffold files (shim and maintenance workflows) to the .fullsend repo without touching secrets, variables, or enrollment. Useful after fullsend version upgrades. Idempotent and safe to run repeatedly.\n\nBy default, changes are delivered via a pull request. Use --direct to push to the default branch instead.", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - if prFlag && directFlag { - return fmt.Errorf("--pr and --direct are mutually exclusive") - } - org := args[0] if err := validateOrgName(org); err != nil { return err @@ -956,16 +951,11 @@ func newGitHubSyncScaffoldCmd() *cobra.Command { printer := ui.New(os.Stdout) // Default is PR delivery; --direct overrides to direct push. - // --pr is accepted for symmetry but is a no-op since it - // matches the default. - direct := directFlag - - return runGitHubSyncScaffold(cmd.Context(), client, printer, org, direct) + return runGitHubSyncScaffold(cmd.Context(), client, printer, org, directFlag) }, } cmd.Flags().BoolVar(&directFlag, "direct", false, "push scaffold files directly to the default branch instead of creating a PR") - cmd.Flags().BoolVar(&prFlag, "pr", false, "deliver changes via a pull request (default behavior)") return cmd } diff --git a/internal/cli/github_test.go b/internal/cli/github_test.go index 6bac824cbb..153fa2bb78 100644 --- a/internal/cli/github_test.go +++ b/internal/cli/github_test.go @@ -600,50 +600,30 @@ func TestRunGitHubSyncScaffold_DefaultCreatesPR(t *testing.T) { assert.Empty(t, client.CommittedFiles, "expected no direct commits when using PR delivery") } -func TestGitHubSyncScaffoldCmd_HasDeliveryFlags(t *testing.T) { +func TestGitHubSyncScaffoldCmd_HasDirectFlag(t *testing.T) { cmd := newGitHubSyncScaffoldCmd() directFlag := cmd.Flags().Lookup("direct") require.NotNil(t, directFlag, "expected --direct flag") assert.Equal(t, "false", directFlag.DefValue) - prFlag := cmd.Flags().Lookup("pr") - require.NotNil(t, prFlag, "expected --pr flag") - assert.Equal(t, "false", prFlag.DefValue) + // --pr flag should not exist; PR delivery is the default. + assert.Nil(t, cmd.Flags().Lookup("pr"), "unexpected --pr flag; PR delivery is the default") } -func TestGitHubSyncScaffoldCmd_MutualExclusiveFlags(t *testing.T) { - t.Setenv("GH_TOKEN", "test-token") - cmd := newRootCmd() - cmd.SetArgs([]string{"github", "sync-scaffold", "--pr", "--direct", "acme"}) - err := cmd.Execute() - require.Error(t, err) - assert.Contains(t, err.Error(), "--pr and --direct are mutually exclusive") -} - -func TestGitHubEnrollCmd_HasDeliveryFlags(t *testing.T) { +func TestGitHubEnrollCmd_HasDirectFlag(t *testing.T) { cmd := newGitHubEnrollCmd() - prFlag := cmd.Flags().Lookup("pr") - require.NotNil(t, prFlag, "expected --pr flag") - assert.Equal(t, "false", prFlag.DefValue) directFlag := cmd.Flags().Lookup("direct") require.NotNil(t, directFlag, "expected --direct flag") assert.Equal(t, "false", directFlag.DefValue) + // --pr flag should not exist; PR delivery is the default. + assert.Nil(t, cmd.Flags().Lookup("pr"), "unexpected --pr flag; PR delivery is the default") } -func TestGitHubUnenrollCmd_HasDeliveryFlags(t *testing.T) { +func TestGitHubUnenrollCmd_HasDirectFlag(t *testing.T) { cmd := newGitHubUnenrollCmd() - prFlag := cmd.Flags().Lookup("pr") - require.NotNil(t, prFlag, "expected --pr flag") directFlag := cmd.Flags().Lookup("direct") require.NotNil(t, directFlag, "expected --direct flag") -} - -func TestGitHubEnrollCmd_MutualExclusiveFlags(t *testing.T) { - t.Setenv("GH_TOKEN", "test-token") - cmd := newRootCmd() - cmd.SetArgs([]string{"github", "enroll", "--pr", "--direct", "--all", "acme"}) - err := cmd.Execute() - require.Error(t, err) - assert.Contains(t, err.Error(), "--pr and --direct are mutually exclusive") + // --pr flag should not exist; PR delivery is the default. + assert.Nil(t, cmd.Flags().Lookup("pr"), "unexpected --pr flag; PR delivery is the default") } func TestRunGitHubSetupPerOrg_DryRun(t *testing.T) { From 326792a0b06514697e9877da548ba0341585304c Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Mon, 29 Jun 2026 11:34:55 -0400 Subject: [PATCH 3/3] fix(cli): use separate branch for enrollment config PRs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address three review findings: 1. saveRepoConfigViaPR now uses fullsend/enrollment-config instead of sharing fullsend/scaffold-install with sync-scaffold, preventing unrelated scaffold and enrollment changes from colliding on the same branch and PR. 2. Skip syncOrgVariableVisibility in PR mode — variable visibility should not change until the enrollment config PR is merged. repo-maintenance reconciles on merge. 3. Remove duplicate godoc line on saveRepoConfig. Assisted-by: Claude Signed-off-by: Wayne Sun --- e2e/admin/admin_test.go | 2 +- internal/cli/admin.go | 19 ++++---- internal/cli/admin_test.go | 2 +- internal/cli/github_test.go | 4 +- internal/layers/commit.go | 75 ++++++++++++++++++++++++++++--- internal/layers/workflows_test.go | 2 +- 6 files changed, 83 insertions(+), 21 deletions(-) diff --git a/e2e/admin/admin_test.go b/e2e/admin/admin_test.go index 525b792921..d3b9a77d3c 100644 --- a/e2e/admin/admin_test.go +++ b/e2e/admin/admin_test.go @@ -826,7 +826,7 @@ func runUnenrollmentTest(t *testing.T, env *e2eEnv) { // watches the repo-maintenance workflow to completion before returning, // so the removal PR should already exist when this returns. output := runCLI(t, env.binary, env.token, - "admin", "disable", "repos", env.org, testRepo, "--yolo") + "admin", "disable", "repos", env.org, testRepo, "--yolo", "--direct") t.Logf("Disable repos output:\n%s", output) // Always capture the repo-maintenance run's logs. Even when the run diff --git a/internal/cli/admin.go b/internal/cli/admin.go index 235b2acade..13686920ac 100644 --- a/internal/cli/admin.go +++ b/internal/cli/admin.go @@ -2342,7 +2342,8 @@ func runEnableRepos(ctx context.Context, client forge.Client, printer *ui.Printe // Sync org variable visibility so enrolled repos can read dispatch // variables like FULLSEND_MINT_URL. Runs even when changed == 0 to // reconcile a previously failed best-effort sync on re-run. - if cfg.Dispatch.Mode == "oidc-mint" { + // Skipped in PR mode — repo-maintenance reconciles on merge. + if cfg.Dispatch.Mode == "oidc-mint" && !pr { syncOrgVariableVisibility(ctx, client, printer, org, cfg, allOrgRepos) } @@ -2504,7 +2505,8 @@ func runDisableRepos(ctx context.Context, client forge.Client, printer *ui.Print } // Sync org variable visibility to revoke access for disabled repos. - if cfg.Dispatch.Mode == "oidc-mint" { + // Skipped in PR mode — repo-maintenance reconciles on merge. + if cfg.Dispatch.Mode == "oidc-mint" && !pr { allOrgRepos, listErr := client.ListOrgRepos(ctx, org) if listErr != nil { printer.StepWarn(fmt.Sprintf("could not list org repos for variable sync: %v", listErr)) @@ -2568,7 +2570,6 @@ func loadRepoConfig(ctx context.Context, client forge.Client, printer *ui.Printe return cfg, nil } -// saveRepoConfig marshals and commits the updated config, then triggers the repo-maintenance workflow. // saveRepoConfig marshals the config, commits it, and dispatches the // repo-maintenance workflow. It returns the dispatch time so callers can // watch the resulting workflow run. A zero time means the dispatch failed. @@ -2611,9 +2612,8 @@ func saveRepoConfig(ctx context.Context, client forge.Client, printer *ui.Printe return dispatchTime, nil } -// saveRepoConfigViaPR delivers config.yaml via a pull request. Uses the -// same branch/PR pattern as scaffold PR delivery: a fixed branch name so -// re-runs update the same PR rather than creating a new one. +// saveRepoConfigViaPR delivers config.yaml via a pull request on a dedicated +// branch, separate from the scaffold-install branch used by sync-scaffold. func saveRepoConfigViaPR(ctx context.Context, client forge.Client, printer *ui.Printer, org string, configData []byte, commitMsg string) (time.Time, error) { cfgRepo, err := client.GetRepo(ctx, org, forge.ConfigRepoName) if err != nil { @@ -2627,14 +2627,13 @@ func saveRepoConfigViaPR(ctx context.Context, client forge.Client, printer *ui.P Mode: "100644", }} - prTitle := "chore: update fullsend enrollment config" prBody := "This PR updates `config.yaml` in the .fullsend config repo.\n\n" + "Merge this PR to apply the enrollment changes. The repo-maintenance workflow will run automatically on merge." - printer.StepStart("Creating enrollment config PR") - _, prErr := layers.CommitScaffoldFiles(ctx, client, printer, + _, prErr := layers.CommitFilesViaPR(ctx, client, printer, org, forge.ConfigRepoName, cfgRepo.DefaultBranch, - commitMsg, prTitle, prBody, files, false) + "fullsend/enrollment-config", + commitMsg, commitMsg, prBody, files) if prErr != nil { return time.Time{}, prErr } diff --git a/internal/cli/admin_test.go b/internal/cli/admin_test.go index 94ad440d59..234ccd6ba3 100644 --- a/internal/cli/admin_test.go +++ b/internal/cli/admin_test.go @@ -2667,7 +2667,7 @@ func TestApplyPerRepoScaffold_ProtectedBranch_ScaffoldBranchAlsoProtected(t *tes err := applyPerRepoScaffold(context.Background(), client, printer, "acme", "widget", files, nil, nil, true) require.Error(t, err) - assert.Contains(t, err.Error(), "scaffold branch") + assert.Contains(t, err.Error(), "is protected") assert.Contains(t, err.Error(), "configure branch protection") } diff --git a/internal/cli/github_test.go b/internal/cli/github_test.go index 153fa2bb78..ecc181a893 100644 --- a/internal/cli/github_test.go +++ b/internal/cli/github_test.go @@ -585,9 +585,9 @@ func TestRunGitHubSyncScaffold_InvalidConfig(t *testing.T) { func TestRunGitHubSyncScaffold_DefaultCreatesPR(t *testing.T) { client := forge.NewFakeClient() client.Repos = []forge.Repository{ - {Name: ".fullsend", FullName: "acme/.fullsend"}, + {Name: ".fullsend", FullName: "acme/.fullsend", DefaultBranch: "main"}, } - client.AuthenticatedUser = "testuser" + client.AuthenticatedUser = "acme" printer := ui.New(&discardWriter{}) // direct=false means PR-based delivery (the default). diff --git a/internal/layers/commit.go b/internal/layers/commit.go index 3912e772e5..a5ae2e391c 100644 --- a/internal/layers/commit.go +++ b/internal/layers/commit.go @@ -35,6 +35,18 @@ func CommitScaffoldFiles(ctx context.Context, client forge.Client, printer *ui.P owner, repo, defaultBranch, commitMsg, prTitle, prBody, files, in) } +// CommitFilesViaPR delivers files via a pull request on the given branch. +// Uses a fixed branch name so re-runs update the same PR. +func CommitFilesViaPR(ctx context.Context, client forge.Client, printer *ui.Printer, + owner, repo, defaultBranch, branch, commitMsg, prTitle, prBody string, + files []forge.TreeFile) (bool, error) { + + return commitViaPR(ctx, client, printer, + owner, repo, defaultBranch, branch, commitMsg, prTitle, prBody, files) +} + +const defaultScaffoldBranch = "fullsend/scaffold-install" + // 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. @@ -42,8 +54,6 @@ func commitScaffoldViaPR(ctx context.Context, client forge.Client, printer *ui.P owner, repo, defaultBranch, commitMsg, prTitle, prBody string, files []forge.TreeFile, in io.Reader) (bool, error) { - const scaffoldBranch = "fullsend/scaffold-install" - user, err := client.GetAuthenticatedUser(ctx) if err != nil { return false, fmt.Errorf("getting authenticated user: %w", err) @@ -52,7 +62,7 @@ func commitScaffoldViaPR(ctx context.Context, client forge.Client, printer *ui.P // Owner pushes directly to the repo — no fork needed. if strings.EqualFold(user, owner) { return commitBranchAndPR(ctx, client, printer, - owner, repo, owner, repo, scaffoldBranch, defaultBranch, + owner, repo, owner, repo, defaultScaffoldBranch, defaultBranch, commitMsg, prTitle, prBody, files) } @@ -65,7 +75,7 @@ func commitScaffoldViaPR(ctx context.Context, client forge.Client, printer *ui.P if forkOwner != "" { printer.StepDone(fmt.Sprintf("Using existing fork %s/%s", forkOwner, forkRepo)) return commitViaFork(ctx, client, printer, - owner, repo, forkOwner, forkRepo, scaffoldBranch, defaultBranch, + owner, repo, forkOwner, forkRepo, defaultScaffoldBranch, defaultBranch, commitMsg, prTitle, prBody, files) } @@ -83,13 +93,13 @@ func commitScaffoldViaPR(ctx context.Context, client forge.Client, printer *ui.P if useFork { return forkAndCommit(ctx, client, printer, - owner, repo, scaffoldBranch, defaultBranch, + owner, repo, defaultScaffoldBranch, defaultBranch, commitMsg, prTitle, prBody, files) } // Upstream path: try to push directly, fail clearly on 403. return commitBranchAndPR(ctx, client, printer, - owner, repo, owner, repo, scaffoldBranch, defaultBranch, + owner, repo, owner, repo, defaultScaffoldBranch, defaultBranch, commitMsg, prTitle, prBody, files) } @@ -186,6 +196,59 @@ func commitBranchAndPR(ctx context.Context, client forge.Client, printer *ui.Pri return false, nil } +// commitViaPR creates a feature branch, commits files, and opens a PR. +// Unlike commitBranchAndPR, this is a simpler pathway for same-owner PRs +// (e.g., enrollment config updates) that don't need fork support. +func commitViaPR(ctx context.Context, client forge.Client, printer *ui.Printer, + owner, repo, defaultBranch, branch, commitMsg, prTitle, prBody string, + files []forge.TreeFile) (bool, error) { + + if branchErr := client.CreateBranch(ctx, owner, repo, branch); branchErr != nil { + if forge.IsForbidden(branchErr) { + printer.StepFail("Insufficient permissions to create branch") + return false, fmt.Errorf("cannot push to %s/%s (403 forbidden); check your token scopes: %w", + owner, repo, branchErr) + } + if !forge.IsAlreadyExists(branchErr) { + printer.StepFail("Failed to create feature branch") + return false, fmt.Errorf("creating feature branch: %w", branchErr) + } + } + + branchCommitted, commitErr := client.CommitFilesToBranch(ctx, owner, repo, branch, commitMsg, files) + if commitErr != nil { + if forge.IsBranchProtected(commitErr) { + printer.StepFail("Feature branch is protected — cannot commit") + return false, fmt.Errorf("branch %q is protected; configure branch protection to allow pushes: %w", branch, commitErr) + } + printer.StepFail("Failed to commit files to branch") + return false, fmt.Errorf("committing files to branch: %w", commitErr) + } + + proposal, prErr := client.CreateChangeProposal(ctx, owner, repo, + prTitle, prBody, branch, defaultBranch) + if prErr != nil { + if forge.IsNoChanges(prErr) { + printer.StepDone("Branch and PR up to date") + return false, nil + } + if !forge.IsAlreadyExists(prErr) { + printer.StepFail("Failed to create PR") + return false, fmt.Errorf("creating PR: %w", prErr) + } + if branchCommitted { + printer.StepDone("PR already exists — updated with new files") + printer.StepInfo("Merge the PR to apply changes") + } else { + printer.StepDone("Branch and PR up to date") + } + } else { + printer.StepDone(fmt.Sprintf("Created PR #%d: %s", proposal.Number, proposal.URL)) + printer.StepInfo("Merge the PR to apply changes") + } + return false, nil +} + // waitForFork polls GetRepo until the fork is ready or the timeout expires. // GitHub fork creation is async (202 Accepted) and can take up to several // minutes for large repos. diff --git a/internal/layers/workflows_test.go b/internal/layers/workflows_test.go index 9349609889..12d04fb486 100644 --- a/internal/layers/workflows_test.go +++ b/internal/layers/workflows_test.go @@ -357,7 +357,7 @@ func TestWorkflowsLayer_Install_ProtectedBranch_ScaffoldBranchAlsoProtected(t *t err := layer.Install(context.Background()) require.Error(t, err) - assert.Contains(t, err.Error(), "scaffold branch") + assert.Contains(t, err.Error(), "is protected") assert.Contains(t, err.Error(), "configure branch protection") }