diff --git a/internal/cli/admin.go b/internal/cli/admin.go index d0a39235f..4fc6337c9 100644 --- a/internal/cli/admin.go +++ b/internal/cli/admin.go @@ -40,6 +40,9 @@ import ( // the --mint-url flag. const DefaultMintURL = "https://fullsend-mint-gljhbkcloq-uc.a.run.app" +const defaultScaffoldPRBody = "This PR adds the fullsend scaffold files for per-repo installation.\n\n" + + "Merge this PR to activate fullsend workflows." + func newAdminCmd() *cobra.Command { cmd := &cobra.Command{ Use: "admin", @@ -1014,10 +1017,9 @@ func runPerRepoInstall(ctx context.Context, c perRepoInstallConfig) error { } return fmt.Errorf("getting repo info: %w", repoErr) } - commitMsg := fmt.Sprintf("chore: initialize fullsend-%s per-repo installation", version) + commitMsg := "chore: initialize fullsend per-repo installation" prTitle := "chore: initialize fullsend per-repo installation" - prBody := "This PR adds the fullsend scaffold files for per-repo installation.\n\n" + - "Merge this PR to activate fullsend workflows." + prBody := defaultScaffoldPRBody if direct { printer.StepStart(fmt.Sprintf("Committing scaffold files to %s/%s (%s branch)", owner, repo, targetRepo.DefaultBranch)) @@ -1200,7 +1202,7 @@ func applyPerRepoScaffold(ctx context.Context, client forge.Client, printer *ui. } return fmt.Errorf("getting repo info: %w", err) } - commitMsg := fmt.Sprintf("chore: initialize fullsend-%s per-repo installation", version) + commitMsg := "chore: initialize fullsend per-repo installation" if direct { printer.StepStart(fmt.Sprintf("Committing scaffold files to %s/%s (%s branch)", owner, repo, targetRepo.DefaultBranch)) @@ -1208,11 +1210,9 @@ func applyPerRepoScaffold(ctx context.Context, client forge.Client, printer *ui. printer.StepStart(fmt.Sprintf("Creating scaffold PR for %s/%s (target: %s)", owner, repo, targetRepo.DefaultBranch)) } - prBody := "This PR adds the fullsend scaffold files for per-repo installation.\n\n" + - "Merge this PR to activate fullsend workflows." if _, err := layers.CommitScaffoldFiles(ctx, client, printer, owner, repo, targetRepo.DefaultBranch, - commitMsg, "chore: initialize fullsend per-repo installation", prBody, files, direct, os.Stdin); err != nil { + commitMsg, "chore: initialize fullsend per-repo installation", defaultScaffoldPRBody, files, direct, os.Stdin); err != nil { return err } diff --git a/internal/cli/repos.go b/internal/cli/repos.go index 06df1caae..413a4232d 100644 --- a/internal/cli/repos.go +++ b/internal/cli/repos.go @@ -421,8 +421,7 @@ func runReposInstall(ctx context.Context, opts *reposInstallConfig) error { } commitMsg := "chore: initialize fullsend per-repo installation" prTitle := "chore: initialize fullsend per-repo installation" - prBody := "This PR adds the fullsend scaffold files for per-repo installation.\n\n" + - "Merge this PR to activate fullsend workflows." + prBody := defaultScaffoldPRBody _, commitErr := layers.CommitScaffoldFiles(ctx, client, printer, owner, repo, targetRepo.DefaultBranch, commitMsg, prTitle, prBody, files, direct, nil) return commitErr diff --git a/internal/layers/commit.go b/internal/layers/commit.go index 58e53a08d..783c4c1c2 100644 --- a/internal/layers/commit.go +++ b/internal/layers/commit.go @@ -5,6 +5,7 @@ import ( "context" "fmt" "io" + "regexp" "strings" "time" @@ -27,6 +28,8 @@ func CommitScaffoldFiles(ctx context.Context, client forge.Client, printer *ui.P owner, repo, defaultBranch, commitMsg, prTitle, prBody string, files []forge.TreeFile, direct bool, in io.Reader) (bool, error) { + commitMsg = adaptCommitMsg(ctx, client, printer, owner, repo, commitMsg) + if direct { return commitScaffoldDirect(ctx, client, printer, owner, repo, defaultBranch, commitMsg, prTitle, prBody, files, in) @@ -66,6 +69,15 @@ func commitScaffoldViaPR(ctx context.Context, client forge.Client, printer *ui.P commitMsg, prTitle, prBody, files) } + // Non-owner with write access can push directly — avoids fork trust + // gates (/ok-to-test) that block CI on cross-fork PRs. + if hasWriteAccess(ctx, client, owner, repo, user) { + printer.StepInfo(fmt.Sprintf("User %s has write access — pushing directly to %s/%s", user, owner, repo)) + return commitBranchAndPR(ctx, client, printer, + owner, repo, owner, repo, defaultScaffoldBranch, defaultBranch, + commitMsg, prTitle, prBody, files) + } + // Non-owner: check for existing fork first. forkOwner, forkRepo, err := client.FindExistingFork(ctx, owner, repo) if err != nil { @@ -421,3 +433,95 @@ func commitScaffoldDirect(ctx context.Context, client forge.Client, printer *ui. return committed, nil } + +// adaptCommitMsg checks the target repo for a .gitlint title-match-regex. If +// the current commit message doesn't match, it tries common conventional-commit +// alternatives (chore:, ci:, build:, bare description). Falls back to warning when no +// alternative matches — we can't fabricate a valid message for an arbitrary regex. +func adaptCommitMsg(ctx context.Context, client forge.Client, printer *ui.Printer, owner, repo, commitMsg string) string { + re := gitlintTitleRegex(ctx, client, owner, repo) + if re == nil { + return commitMsg + } + title := strings.SplitN(commitMsg, "\n", 2)[0] + if re.MatchString(title) { + return commitMsg + } + + desc := title + if idx := strings.Index(title, ": "); idx >= 0 { + desc = title[idx+2:] + } + alternatives := []string{ + "chore: " + desc, + "ci: " + desc, + "build: " + desc, + desc, + } + var body string + if parts := strings.SplitN(commitMsg, "\n", 2); len(parts) == 2 { + body = parts[1] + } + for _, alt := range alternatives { + if alt == title { + continue + } + if re.MatchString(alt) { + adapted := alt + if body != "" { + adapted += "\n" + body + } + printer.StepInfo(fmt.Sprintf("Adapted scaffold commit message to match .gitlint: %q", alt)) + return adapted + } + } + + printer.StepWarn(fmt.Sprintf("Scaffold commit message %q does not match this repo's .gitlint title-match-regex (%s) — commit-lint CI may fail", + title, re.String())) + return commitMsg +} + +// gitlintTitleRegex reads .gitlint from the target repo and extracts the +// title-match-regex value. Returns nil if the file doesn't exist or has no +// title-match-regex rule. +func gitlintTitleRegex(ctx context.Context, client forge.Client, owner, repo string) *regexp.Regexp { + content, err := client.GetFileContent(ctx, owner, repo, ".gitlint") + if err != nil { + return nil + } + inSection := false + for _, line := range strings.Split(string(content), "\n") { + line = strings.TrimSpace(line) + if strings.HasPrefix(line, "[") { + inSection = strings.EqualFold(line, "[title-match-regex]") + continue + } + if !inSection { + continue + } + if parts := strings.SplitN(line, "=", 2); len(parts) == 2 && strings.TrimSpace(parts[0]) == "regex" { + val := strings.TrimSpace(parts[1]) + re, err := regexp.Compile(val) + if err != nil { + return nil + } + return re + } + } + return nil +} + +// hasWriteAccess checks whether the user has write or admin permission on the +// repo. Returns false on any error (or non-GitHub forges) so the caller falls +// through to the fork path. +func hasWriteAccess(ctx context.Context, client forge.Client, owner, repo, user string) bool { + ghExt, ok := client.(forge.GitHubExtensions) + if !ok { + return false + } + role, err := ghExt.GetCollaboratorPermission(ctx, owner, repo, user) + if err != nil { + return false + } + return role == "write" || role == "maintain" || role == "admin" +} diff --git a/internal/layers/commit_test.go b/internal/layers/commit_test.go index f8392d1c1..8b88579ba 100644 --- a/internal/layers/commit_test.go +++ b/internal/layers/commit_test.go @@ -78,6 +78,101 @@ func TestCommitScaffoldViaPR_ExistingForkReused(t *testing.T) { require.Len(t, client.CreatedProposals, 1) } +func TestCommitScaffoldViaPR_WriteAccessPushesDirect(t *testing.T) { + client := forge.NewFakeClient() + client.AuthenticatedUser = "contributor" + client.CollaboratorPermissions = map[string]string{ + "acme/widget/contributor": "write", + } + printer, buf := newTestPrinter() + + _, err := CommitScaffoldFiles(context.Background(), client, printer, + "acme", "widget", "main", "msg", "title", "body", testFiles, false, nil) + require.NoError(t, err) + + assert.Contains(t, buf.String(), "has write access") + assert.Empty(t, client.CreatedForks, "should not fork when user has write access") + require.Len(t, client.CreatedBranches, 1) + assert.Equal(t, "acme/widget/fullsend/scaffold-install", client.CreatedBranches[0]) +} + +func TestCommitScaffoldViaPR_AdminAccessPushesDirect(t *testing.T) { + client := forge.NewFakeClient() + client.AuthenticatedUser = "contributor" + client.CollaboratorPermissions = map[string]string{ + "acme/widget/contributor": "admin", + } + printer, _ := newTestPrinter() + + _, err := CommitScaffoldFiles(context.Background(), client, printer, + "acme", "widget", "main", "msg", "title", "body", testFiles, false, nil) + require.NoError(t, err) + + assert.Empty(t, client.CreatedForks) + require.Len(t, client.CreatedBranches, 1) + assert.Equal(t, "acme/widget/fullsend/scaffold-install", client.CreatedBranches[0]) +} + +func TestCommitScaffoldViaPR_MaintainAccessPushesDirect(t *testing.T) { + client := forge.NewFakeClient() + client.AuthenticatedUser = "contributor" + client.CollaboratorPermissions = map[string]string{ + "acme/widget/contributor": "maintain", + } + printer, buf := newTestPrinter() + + _, err := CommitScaffoldFiles(context.Background(), client, printer, + "acme", "widget", "main", "msg", "title", "body", testFiles, false, nil) + require.NoError(t, err) + + assert.Contains(t, buf.String(), "has write access") + assert.Empty(t, client.CreatedForks) + require.Len(t, client.CreatedBranches, 1) + assert.Equal(t, "acme/widget/fullsend/scaffold-install", client.CreatedBranches[0]) +} + +func TestCommitScaffoldViaPR_WriteAccessTakesPrecedenceOverFork(t *testing.T) { + client := forge.NewFakeClient() + client.AuthenticatedUser = "contributor" + client.CollaboratorPermissions = map[string]string{ + "acme/widget/contributor": "write", + } + client.ExistingForks = map[string]string{ + "acme/widget": "contributor", + } + printer, buf := newTestPrinter() + + _, err := CommitScaffoldFiles(context.Background(), client, printer, + "acme", "widget", "main", "msg", "title", "body", testFiles, false, nil) + require.NoError(t, err) + + assert.Contains(t, buf.String(), "has write access") + assert.NotContains(t, buf.String(), "Using existing fork") + assert.Empty(t, client.CreatedForks) + require.Len(t, client.CreatedBranches, 1) + assert.Equal(t, "acme/widget/fullsend/scaffold-install", client.CreatedBranches[0], + "write access should push to upstream, not the fork") +} + +func TestCommitScaffoldViaPR_ReadAccessFallsThrough(t *testing.T) { + client := forge.NewFakeClient() + client.AuthenticatedUser = "contributor" + client.TokenScopes = []string{"repo", "workflow"} + client.CollaboratorPermissions = map[string]string{ + "acme/widget/contributor": "read", + } + client.Repos = append(client.Repos, forge.Repository{ + FullName: "contributor/widget", DefaultBranch: "main", + }) + printer, _ := newTestPrinter() + + _, err := CommitScaffoldFiles(context.Background(), client, printer, + "acme", "widget", "main", "msg", "title", "body", testFiles, false, nil) + require.NoError(t, err) + + require.Len(t, client.CreatedForks, 1, "read-only user should fork") +} + func TestCommitScaffoldViaPR_NonInteractiveForksByDefault(t *testing.T) { client := forge.NewFakeClient() client.AuthenticatedUser = "contributor" @@ -440,3 +535,143 @@ func TestPromptForkChoice_MaxRetries(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "too many invalid attempts") } + +func TestGitlintTitleRegex(t *testing.T) { + t.Run("no gitlint file", func(t *testing.T) { + client := forge.NewFakeClient() + re := gitlintTitleRegex(context.Background(), client, "acme", "widget") + assert.Nil(t, re) + }) + + t.Run("gitlint with title-match-regex", func(t *testing.T) { + client := forge.NewFakeClient() + client.FileContents["acme/widget/.gitlint"] = []byte( + "[general]\nignore=body-is-missing\n\n[title-match-regex]\nregex=^(feat|fix|chore)(\\(.+\\))?: .+\n") + re := gitlintTitleRegex(context.Background(), client, "acme", "widget") + require.NotNil(t, re) + assert.True(t, re.MatchString("chore: initialize fullsend per-repo installation")) + assert.False(t, re.MatchString("PROJ-123: add stuff")) + }) + + t.Run("gitlint with custom regex requiring ticket prefix", func(t *testing.T) { + client := forge.NewFakeClient() + client.FileContents["acme/widget/.gitlint"] = []byte( + "[title-match-regex]\nregex=^PROJ-\\d+: .+\n") + re := gitlintTitleRegex(context.Background(), client, "acme", "widget") + require.NotNil(t, re) + assert.False(t, re.MatchString("chore: initialize fullsend per-repo installation"), + "conventional commit should not match a ticket-prefix regex") + }) + + t.Run("gitlint without title-match-regex section", func(t *testing.T) { + client := forge.NewFakeClient() + client.FileContents["acme/widget/.gitlint"] = []byte( + "[general]\nignore=body-is-missing\n\n[title-max-length]\nline-length=72\n") + re := gitlintTitleRegex(context.Background(), client, "acme", "widget") + assert.Nil(t, re) + }) + + t.Run("gitlint with spaces around equals", func(t *testing.T) { + client := forge.NewFakeClient() + client.FileContents["acme/widget/.gitlint"] = []byte( + "[title-match-regex]\nregex = ^fix: .+\n") + re := gitlintTitleRegex(context.Background(), client, "acme", "widget") + require.NotNil(t, re) + assert.True(t, re.MatchString("fix: something")) + }) + + t.Run("gitlint with tabs and extra spaces around equals", func(t *testing.T) { + client := forge.NewFakeClient() + client.FileContents["acme/widget/.gitlint"] = []byte( + "[title-match-regex]\nregex\t= ^fix: .+\n") + re := gitlintTitleRegex(context.Background(), client, "acme", "widget") + require.NotNil(t, re) + assert.True(t, re.MatchString("fix: something")) + }) + + t.Run("invalid regex is ignored", func(t *testing.T) { + client := forge.NewFakeClient() + client.FileContents["acme/widget/.gitlint"] = []byte( + "[title-match-regex]\nregex=[invalid((\n") + re := gitlintTitleRegex(context.Background(), client, "acme", "widget") + assert.Nil(t, re) + }) +} + +func TestAdaptCommitMsg(t *testing.T) { + t.Run("no gitlint warns nothing", func(t *testing.T) { + client := forge.NewFakeClient() + printer, buf := newTestPrinter() + msg := adaptCommitMsg(context.Background(), client, printer, "acme", "widget", + "chore: initialize fullsend per-repo installation") + assert.Equal(t, "chore: initialize fullsend per-repo installation", msg) + assert.NotContains(t, buf.String(), "gitlint") + }) + + t.Run("matching regex warns nothing", func(t *testing.T) { + client := forge.NewFakeClient() + client.FileContents["acme/widget/.gitlint"] = []byte( + "[title-match-regex]\nregex=^(feat|fix|chore): .+\n") + printer, buf := newTestPrinter() + msg := adaptCommitMsg(context.Background(), client, printer, "acme", "widget", + "chore: initialize fullsend per-repo installation") + assert.Equal(t, "chore: initialize fullsend per-repo installation", msg) + assert.NotContains(t, buf.String(), "gitlint") + }) + + t.Run("adapts to ci prefix when chore does not match", func(t *testing.T) { + client := forge.NewFakeClient() + client.FileContents["acme/widget/.gitlint"] = []byte( + "[title-match-regex]\nregex=^(ci|build): .+\n") + printer, buf := newTestPrinter() + msg := adaptCommitMsg(context.Background(), client, printer, "acme", "widget", + "chore: initialize fullsend per-repo installation") + assert.Equal(t, "ci: initialize fullsend per-repo installation", msg) + assert.Contains(t, buf.String(), "Adapted scaffold commit message") + assert.NotContains(t, buf.String(), "CI may fail") + }) + + t.Run("adapts to bare description when no prefix matches", func(t *testing.T) { + client := forge.NewFakeClient() + client.FileContents["acme/widget/.gitlint"] = []byte( + "[title-match-regex]\nregex=^[a-z]+ .+\n") + printer, buf := newTestPrinter() + msg := adaptCommitMsg(context.Background(), client, printer, "acme", "widget", + "chore: initialize fullsend per-repo installation") + assert.Equal(t, "initialize fullsend per-repo installation", msg) + assert.Contains(t, buf.String(), "Adapted scaffold commit message") + }) + + t.Run("warns when no alternative matches", func(t *testing.T) { + client := forge.NewFakeClient() + client.FileContents["acme/widget/.gitlint"] = []byte( + "[title-match-regex]\nregex=^PROJ-\\d+: .+\n") + printer, buf := newTestPrinter() + msg := adaptCommitMsg(context.Background(), client, printer, "acme", "widget", + "chore: initialize fullsend per-repo installation") + assert.Equal(t, "chore: initialize fullsend per-repo installation", msg) + assert.Contains(t, buf.String(), "title-match-regex") + assert.Contains(t, buf.String(), "commit-lint CI may fail") + }) + + t.Run("adapts non-scaffold commit message", func(t *testing.T) { + client := forge.NewFakeClient() + client.FileContents["acme/widget/.gitlint"] = []byte( + "[title-match-regex]\nregex=^(ci|build): .+\n") + printer, buf := newTestPrinter() + msg := adaptCommitMsg(context.Background(), client, printer, "acme", "widget", + "chore: upgrade fullsend config") + assert.Equal(t, "ci: upgrade fullsend config", msg) + assert.Contains(t, buf.String(), "Adapted scaffold commit message") + }) + + t.Run("preserves body when adapting", func(t *testing.T) { + client := forge.NewFakeClient() + client.FileContents["acme/widget/.gitlint"] = []byte( + "[title-match-regex]\nregex=^(ci|build): .+\n") + printer, _ := newTestPrinter() + msg := adaptCommitMsg(context.Background(), client, printer, "acme", "widget", + "chore: initialize fullsend per-repo installation\n\nSigned-off-by: bot") + assert.Equal(t, "ci: initialize fullsend per-repo installation\n\nSigned-off-by: bot", msg) + }) +} diff --git a/internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml b/internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml index 049e80882..4762047d9 100644 --- a/internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml +++ b/internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml @@ -38,8 +38,10 @@ on: jobs: dispatch: if: >- - github.event_name != 'issue_comment' - || github.event.comment.user.type != 'Bot' + (github.event_name != 'pull_request_target' && github.event_name != 'pull_request_review' + || github.event.pull_request.head.ref != 'fullsend/scaffold-install') + && (github.event_name != 'issue_comment' + || github.event.comment.user.type != 'Bot') uses: __REUSABLE_DISPATCH__ with: event_action: ${{ github.event.action }} diff --git a/internal/scaffold/fullsend-repo/templates/shim-workflow-call.yaml b/internal/scaffold/fullsend-repo/templates/shim-workflow-call.yaml index bac7b6281..d49b0cf09 100644 --- a/internal/scaffold/fullsend-repo/templates/shim-workflow-call.yaml +++ b/internal/scaffold/fullsend-repo/templates/shim-workflow-call.yaml @@ -41,8 +41,10 @@ jobs: group: fullsend-dispatch-${{ github.event.issue.number || github.event.pull_request.number }} cancel-in-progress: false if: >- - github.event_name != 'issue_comment' - || github.event.comment.user.type != 'Bot' + (github.event_name != 'pull_request_target' && github.event_name != 'pull_request_review' + || github.event.pull_request.head.ref != 'fullsend/scaffold-install') + && (github.event_name != 'issue_comment' + || github.event.comment.user.type != 'Bot') uses: __ORG__/.fullsend/.github/workflows/dispatch.yml@main with: event_action: ${{ github.event.action }} diff --git a/internal/scaffold/workflow_call_alignment_test.go b/internal/scaffold/workflow_call_alignment_test.go index 0cdacff9e..1e491d226 100644 --- a/internal/scaffold/workflow_call_alignment_test.go +++ b/internal/scaffold/workflow_call_alignment_test.go @@ -505,6 +505,26 @@ func TestDispatchPerStageAuthorization(t *testing.T) { } } +// TestShimScaffoldBranchFilter validates that both shim templates skip dispatch +// for PRs from the fullsend/scaffold branch. Without this filter, the shim +// fires pull_request_target on the scaffold PR, causing dispatch noise (#5470). +func TestShimScaffoldBranchFilter(t *testing.T) { + cases := []struct { + name string + content func(t *testing.T) []byte + }{ + {"shim-workflow-call", loadScaffoldFile("templates/shim-workflow-call.yaml")}, + {"shim-per-repo", loadScaffoldFile("templates/shim-per-repo.yaml")}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + s := string(tc.content(t)) + assert.Contains(t, s, "github.event.pull_request.head.ref != 'fullsend/scaffold-install'", + "%s dispatch job must filter scaffold branch PRs to prevent self-dispatch noise", tc.name) + }) + } +} + // TestDispatchPRHeadResolution validates that both dispatch workflows contain // the "Resolve PR head for issue_comment events" step and the pull_request // merge into event_payload, ensuring issue_comment-triggered agents receive