diff --git a/modules/actions/workflows.go b/modules/actions/workflows.go index d5886309f73..dc7c55b9c04 100644 --- a/modules/actions/workflows.go +++ b/modules/actions/workflows.go @@ -35,7 +35,7 @@ type detectResult int const ( detectMatched detectResult = iota // event matched; run normally detectNotApplicable // event/type doesn't apply; create nothing - detectFilteredOut // matched but excluded by a branch/paths filter; emits a skipped commit status + detectFilteredOut // matched but excluded by a branch/paths filter; posts a skipped commit status when the context is a required check ) func init() { diff --git a/services/actions/commit_status.go b/services/actions/commit_status.go index 2d2f0f051f5..dc999f323d6 100644 --- a/services/actions/commit_status.go +++ b/services/actions/commit_status.go @@ -7,6 +7,7 @@ import ( "context" "errors" "fmt" + "slices" actions_model "gitea.dev/models/actions" "gitea.dev/models/db" @@ -16,10 +17,12 @@ import ( actions_module "gitea.dev/modules/actions" "gitea.dev/modules/actions/jobparser" "gitea.dev/modules/commitstatus" + "gitea.dev/modules/glob" "gitea.dev/modules/log" api "gitea.dev/modules/structs" "gitea.dev/modules/util" webhook_module "gitea.dev/modules/webhook" + pull_service "gitea.dev/services/pull" commitstatus_service "gitea.dev/services/repository/commitstatus" ) @@ -153,12 +156,43 @@ func createCommitStatus(ctx context.Context, repo *repo_model.Repository, event, return createWorkflowCommitStatus(ctx, repo, commitID, ctxName, run.WorkflowID, toCommitStatus(job.Status), targetURL, toCommitStatusDescription(job)) } +// getAllRequiredStatusContextGlobs returns the compiled globs of every status-check context required in the repo: +// its protected branch rules' contexts and its required scoped workflows' patterns (see EffectiveRequiredContexts). +func getAllRequiredStatusContextGlobs(ctx context.Context, repo *repo_model.Repository) ([]glob.Glob, error) { + rules, err := git_model.FindRepoProtectedBranchRules(ctx, repo.ID) + if err != nil { + return nil, fmt.Errorf("FindRepoProtectedBranchRules: %w", err) + } + required, err := pull_service.EffectiveRequiredContexts(ctx, repo, rules...) + if err != nil { + return nil, fmt.Errorf("EffectiveRequiredContexts: %w", err) + } + globs := make([]glob.Glob, 0, len(required)) + for _, c := range required { + if gp, err := glob.Compile(c); err != nil { + log.Error("glob.Compile %q: %v", c, err) + } else { + globs = append(globs, gp) + } + } + return globs, nil +} + // CreateSkippedCommitStatusForFilteredWorkflow posts a skipped commit status for each job of a // workflow that matched the triggering event but was excluded by a branch/paths filter. // This lets a required status check tied to that context be satisfied without the workflow running. +// Only contexts matching requiredGlobs are posted; a non-required context gets no skipped status. // No ActionRun is created, so the status has no target URL (there is no run/job to link to). // A non-empty scopedPrefix prefixes each context with its source repo, matching scoped runs. -func CreateSkippedCommitStatusForFilteredWorkflow(ctx context.Context, repo *repo_model.Repository, event webhook_module.HookEventType, triggerEvent, workflowID string, content []byte, payload api.Payloader, scopedPrefix string) error { +// +// TODO: requiredGlobs over-approximates by including every protected branch rule's required contexts. +// If possible, the set should be narrowed to the required contexts of a specific branch protection rule (and the contexts from required scoped workflows). +// Currently, a `push` event must keep the repo-wide union since its future pull request base branch is unknown. +func CreateSkippedCommitStatusForFilteredWorkflow(ctx context.Context, repo *repo_model.Repository, event webhook_module.HookEventType, triggerEvent, workflowID string, content []byte, payload api.Payloader, scopedPrefix string, requiredGlobs []glob.Glob) error { + if len(requiredGlobs) == 0 { + return nil // nothing is required, so no skipped status is needed + } + // Derive the status event name and target commit from the payload. // TODO: this mirrors getCommitStatusEventNameAndCommitID, which derives the same from a persisted run. Should merge the logic if possible. var statusEvent, commitID string @@ -200,6 +234,10 @@ func CreateSkippedCommitStatusForFilteredWorkflow(ctx context.Context, repo *rep if scopedPrefix != "" { ctxName = actions_module.ScopedWorkflowStatusContextName(scopedPrefix, displayName, jobName, statusEvent) } + // Only a required context needs the skipped status. + if !slices.ContainsFunc(requiredGlobs, func(gp glob.Glob) bool { return gp.Match(ctxName) }) { + continue + } // "Skipped" mirrors toCommitStatusDescription for StatusSkipped. if err := createWorkflowCommitStatus(ctx, repo, commitID, ctxName, workflowID, commitstatus.CommitStatusSkipped, "", "Skipped"); err != nil { return err diff --git a/services/actions/notifier_helper.go b/services/actions/notifier_helper.go index 20f8a19467a..9e558e2d2f9 100644 --- a/services/actions/notifier_helper.go +++ b/services/actions/notifier_helper.go @@ -396,10 +396,22 @@ func buildApproveAndInsertRun( return nil } -// handleFilteredWorkflows posts a skipped commit status for each workflow that matched the event but was excluded by a branch/paths filter. +// handleFilteredWorkflows posts a skipped commit status for each filtered-out workflow whose context is a required status check; +// a non-required one posts nothing, so it cannot leak into a pull request. func handleFilteredWorkflows(ctx context.Context, input *notifyInput, filteredWorkflows []*actions_module.DetectedWorkflow) { + if len(filteredWorkflows) == 0 { + return + } + requiredGlobs, err := getAllRequiredStatusContextGlobs(ctx, input.Repo) + if err != nil { + log.Error("repo %s: required status contexts: %v", input.Repo.RelativePath(), err) + return + } + if len(requiredGlobs) == 0 { + return + } for _, dwf := range filteredWorkflows { - if err := CreateSkippedCommitStatusForFilteredWorkflow(ctx, input.Repo, input.Event, dwf.TriggerEvent.Name, dwf.EntryName, dwf.Content, input.Payload, ""); err != nil { + if err := CreateSkippedCommitStatusForFilteredWorkflow(ctx, input.Repo, input.Event, dwf.TriggerEvent.Name, dwf.EntryName, dwf.Content, input.Payload, "", requiredGlobs); err != nil { log.Error("repo %s: skipped commit status for workflow %s: %v", input.Repo.RelativePath(), dwf.EntryName, err) continue } @@ -653,6 +665,12 @@ func detectAndHandleScopedWorkflows( isForkPullRequest := isForkPullRequestInput(input) actionsConfig := input.Repo.MustGetUnit(ctx, unit_model.TypeActions).ActionsConfig() + // A filtered-out scoped workflow only posts a skipped status when its context is a required check. + requiredGlobs, err := getAllRequiredStatusContextGlobs(ctx, input.Repo) + if err != nil { + log.Error("scoped workflows: required status contexts for %s: %v", input.Repo.RelativePath(), err) + } + // The same source repo may be registered at both the owner and instance level; dedup // the IDs and batch-load them in one query instead of one round-trip per source. seen := make(container.Set[int64], len(sources)) @@ -696,14 +714,14 @@ func detectAndHandleScopedWorkflows( } } - // A filtered-out scoped workflow posts a skipped commit status. - if len(filtered) > 0 { + // A filtered-out scoped workflow posts a skipped commit status for its required-check contexts. + if len(filtered) > 0 && len(requiredGlobs) > 0 { scopedPrefix := actions_model.ScopedStatusContextPrefix(ctx, sourceRepo.ID) for _, dwf := range filtered { if actions_model.ScopedWorkflowOptedOut(actionsConfig, sources, sourceRepo.ID, dwf.EntryName) { continue } - if err := CreateSkippedCommitStatusForFilteredWorkflow(ctx, input.Repo, input.Event, dwf.TriggerEvent.Name, dwf.EntryName, dwf.Content, input.Payload, scopedPrefix); err != nil { + if err := CreateSkippedCommitStatusForFilteredWorkflow(ctx, input.Repo, input.Event, dwf.TriggerEvent.Name, dwf.EntryName, dwf.Content, input.Payload, scopedPrefix, requiredGlobs); err != nil { log.Error("scoped workflows: skipped commit status for source %s workflow %s: %v", sourceRepo.RelativePath(), dwf.EntryName, err) continue } @@ -716,7 +734,7 @@ func detectAndHandleScopedWorkflows( // detectScopedWorkflowsForSource detects the scoped workflows from the source repo at its default branch. // detected are the workflows to run; filtered matched the event but were excluded by a branch/paths -// filter and post a skipped commit status. +// filter, and later post a skipped commit status only for a required-check context. func detectScopedWorkflowsForSource( ctx context.Context, input *notifyInput, diff --git a/services/pull/commit_status.go b/services/pull/commit_status.go index 948e26aaa54..9a2e7b8441b 100644 --- a/services/pull/commit_status.go +++ b/services/pull/commit_status.go @@ -156,11 +156,16 @@ func GetPullRequestCommitStatusState(ctx context.Context, pr *issues_model.PullR return MergeRequiredContextsCommitStatus(commitStatuses, requiredContexts), nil } -// EffectiveRequiredContexts returns the required status-check contexts for a PR head: +// EffectiveRequiredContexts returns the required status-check contexts to enforce, drawn from: // 1. every required scoped workflow's status-check patterns effective for the repo (always) -// 2. the branch protection's own configured contexts, only when its status check is enabled -func EffectiveRequiredContexts(ctx context.Context, repo *repo_model.Repository, pb *git_model.ProtectedBranch) ([]string, error) { - if pb == nil { +// 2. each given protected branch rule's own configured contexts, only when that rule's status check is enabled +// +// Passing no rule or a single nil rule yields nothing, not even scoped patterns. +// A single rule yields that rule's effective contexts. +// Passing several rules unions their effective contexts; this is used when the governing rule is not yet known. +func EffectiveRequiredContexts(ctx context.Context, repo *repo_model.Repository, pbs ...*git_model.ProtectedBranch) ([]string, error) { + // No protection rule in effect: nothing is required. + if len(pbs) == 0 || (len(pbs) == 1 && pbs[0] == nil) { return nil, nil } @@ -169,36 +174,28 @@ func EffectiveRequiredContexts(ctx context.Context, repo *repo_model.Repository, return nil, fmt.Errorf("GetEffectiveScopedWorkflowSources: %w", err) } + required := make(container.Set[string]) + // Every required scoped workflow's admin-authored status-check patterns, matched must-present-and-pass: // a required scoped check that posts no matching status blocks the merge. - seen := make(container.Set[string]) - var scoped []string for _, source := range sources { for _, cfg := range source.WorkflowConfigs { if !cfg.Required { continue } - for _, p := range cfg.Patterns { - if seen.Add(p) { - scoped = append(scoped, p) - } - } + required.AddMultiple(cfg.Patterns...) } } - slices.Sort(scoped) // sort for stable output - - // With the branch protection's own status check disabled, only the required scoped checks (mandated by the owner or instance admin) gate the merge. - if !pb.EnableStatusCheck { - return scoped, nil - } - - // Status check enabled: the rule's configured contexts, then the scoped patterns not already among them. - required := slices.Clone(pb.StatusCheckContexts) - for _, p := range scoped { - if !slices.Contains(pb.StatusCheckContexts, p) { - required = append(required, p) + // Union the configured contexts of every rule whose own status check is enabled (a disabled rule contributes none) + for _, pb := range pbs { + if pb == nil || !pb.EnableStatusCheck { + continue } + required.AddMultiple(pb.StatusCheckContexts...) } - return required, nil + + values := required.Values() + slices.Sort(values) // stable output + return values, nil } diff --git a/services/pull/commit_status_test.go b/services/pull/commit_status_test.go index 2fda2f256a6..20889a76265 100644 --- a/services/pull/commit_status_test.go +++ b/services/pull/commit_status_test.go @@ -153,4 +153,14 @@ func TestEffectiveRequiredContexts(t *testing.T) { "org/src: ci.yaml / lint (pull_request)", }, got) }) + + t.Run("several rules union their enabled contexts, deduplicated; a disabled rule contributes none", func(t *testing.T) { + noSourceRepo := &repo_model.Repository{ID: consumer.ID, OwnerID: 99999} // no scoped sources, so only the rules' contexts gate + a := &git_model.ProtectedBranch{EnableStatusCheck: true, StatusCheckContexts: []string{"a/check", "shared/check"}} + b := &git_model.ProtectedBranch{EnableStatusCheck: true, StatusCheckContexts: []string{"b/check", "shared/check"}} + off := &git_model.ProtectedBranch{EnableStatusCheck: false, StatusCheckContexts: []string{"off/check"}} + got, err := EffectiveRequiredContexts(t.Context(), noSourceRepo, a, b, off) + require.NoError(t, err) + assert.ElementsMatch(t, []string{"a/check", "b/check", "shared/check"}, got) + }) } diff --git a/tests/integration/actions_trigger_test.go b/tests/integration/actions_trigger_test.go index adcc403ba22..aa11da83ad9 100644 --- a/tests/integration/actions_trigger_test.go +++ b/tests/integration/actions_trigger_test.go @@ -166,6 +166,14 @@ jobs: assert.Equal(t, addFileToForkedResp.Commit.SHA, actionRun.CommitSHA) assert.Equal(t, actions_module.GithubEventPullRequestTarget, actionRun.TriggerEvent) + // require the workflow's status check on the base branch, so a filtered-out run posts a skipped status to satisfy it + require.NoError(t, git_model.UpdateProtectBranch(t.Context(), baseRepo, &git_model.ProtectedBranch{ + RepoID: baseRepo.ID, + RuleName: "main", + EnableStatusCheck: true, + StatusCheckContexts: []string{"*"}, + }, git_model.WhitelistOptions{})) + // add another file whose name cannot match the specified path addFileToForkedResp, err = files_service.ChangeRepoFiles(t.Context(), forkedRepo, user4, &files_service.ChangeRepoFilesOptions{ Files: []*files_service.ChangeRepoFile{ @@ -218,6 +226,65 @@ jobs: // the new pull request is filtered by paths, so no run is created; a skipped commit status is posted instead assert.Equal(t, 1, unittest.GetCount(t, &actions_model.ActionRun{RepoID: baseRepo.ID})) assertSkippedCommitStatusExists(t, baseRepo.ID, addFileToForkedResp.Commit.SHA, "pull_request_target") + + // delete the branch protection rule: with nothing required, a filtered-out run must post no skipped status + pb, err := git_model.GetProtectedBranchRuleByName(t.Context(), baseRepo.ID, "main") + require.NoError(t, err) + require.NotNil(t, pb) + require.NoError(t, git_model.DeleteProtectedBranch(t.Context(), baseRepo, pb.ID)) + + // add another file whose name cannot match the specified path + addFileToForkedResp, err = files_service.ChangeRepoFiles(t.Context(), forkedRepo, user4, &files_service.ChangeRepoFilesOptions{ + Files: []*files_service.ChangeRepoFile{ + { + Operation: "create", + TreePath: "bar.txt", + ContentReader: strings.NewReader("bar"), + }, + }, + Message: "add bar.txt", + OldBranch: "main", + NewBranch: "fork-branch-3", + Author: &files_service.IdentityOptions{ + GitUserName: user4.Name, + GitUserEmail: user4.Email, + }, + Committer: &files_service.IdentityOptions{ + GitUserName: user4.Name, + GitUserEmail: user4.Email, + }, + Dates: &files_service.CommitDateOptions{ + Author: time.Now(), + Committer: time.Now(), + }, + }) + assert.NoError(t, err) + assert.NotEmpty(t, addFileToForkedResp) + + // create Pull + pullIssue = &issues_model.Issue{ + RepoID: baseRepo.ID, + Title: "A mismatched path with no required status check posts no skipped status", + PosterID: user4.ID, + Poster: user4, + IsPull: true, + } + pullRequest = &issues_model.PullRequest{ + HeadRepoID: forkedRepo.ID, + BaseRepoID: baseRepo.ID, + HeadBranch: "fork-branch-3", + BaseBranch: "main", + HeadRepo: forkedRepo, + BaseRepo: baseRepo, + Type: issues_model.PullRequestGitea, + } + prOpts = &pull_service.NewPullRequestOptions{Repo: baseRepo, Issue: pullIssue, PullRequest: pullRequest} + err = pull_service.NewPullRequest(t.Context(), prOpts) + assert.NoError(t, err) + + // filtered by paths and no required status check remains, so no run and no skipped commit status + assert.Equal(t, 1, unittest.GetCount(t, &actions_model.ActionRun{RepoID: baseRepo.ID})) + assertNoSkippedCommitStatusExists(t, baseRepo.ID, addFileToForkedResp.Commit.SHA, "pull_request_target") }) } @@ -339,9 +406,10 @@ jobs: }) assert.NoError(t, err) assert.NotEmpty(t, addFileToBranchResp) - // the push to test-skip-ci is filtered by branches, so no run is created; a skipped commit status is posted instead + // the push to test-skip-ci is filtered by branches, so no run is created; + // its context is not a required status check either, so no skipped commit status is posted. assert.Equal(t, 1, unittest.GetCount(t, &actions_model.ActionRun{RepoID: repo.ID})) - assertSkippedCommitStatusExists(t, repo.ID, addFileToBranchResp.Commit.SHA, "push") + assertNoSkippedCommitStatusExists(t, repo.ID, addFileToBranchResp.Commit.SHA, "push") resp := testPullCreate(t, session, "user2", "skip-ci", true, "master", "test-skip-ci", "[skip ci] test-skip-ci") @@ -1884,15 +1952,28 @@ jobs: }) } -// assertSkippedCommitStatusExists asserts that a filtered-out workflow posted a skipped commit status on sha +// assertSkippedCommitStatusExists asserts that a filtered-out required workflow posted a skipped commit status on sha func assertSkippedCommitStatusExists(t *testing.T, repoID int64, sha, eventSuffix string) { + t.Helper() + assert.Truef(t, hasSkippedCommitStatus(t, repoID, sha, eventSuffix), "missing skipped commit status with event %q on %s", eventSuffix, sha) +} + +// assertNoSkippedCommitStatusExists asserts that no skipped commit status for the given event was posted on sha, +// e.g. a filtered-out workflow whose context is not a required status check must not leave one behind. +func assertNoSkippedCommitStatusExists(t *testing.T, repoID int64, sha, eventSuffix string) { + t.Helper() + assert.Falsef(t, hasSkippedCommitStatus(t, repoID, sha, eventSuffix), "unexpected skipped commit status with event %q on %s", eventSuffix, sha) +} + +// hasSkippedCommitStatus reports whether a skipped commit status for the given event was posted on sha. +func hasSkippedCommitStatus(t *testing.T, repoID int64, sha, eventSuffix string) bool { t.Helper() statuses, err := git_model.GetLatestCommitStatus(t.Context(), repoID, sha, db.ListOptionsAll) require.NoError(t, err) for _, s := range statuses { if s.State == commitstatus.CommitStatusSkipped && strings.Contains(s.Context, "("+eventSuffix+")") { - return + return true } } - assert.Failf(t, "missing skipped commit status", "no skipped commit status with event %q on %s (found %d statuses)", eventSuffix, sha, len(statuses)) + return false }