diff --git a/models/git/commit_status.go b/models/git/commit_status.go index 918e45c207..fa98cba7ba 100644 --- a/models/git/commit_status.go +++ b/models/git/commit_status.go @@ -505,13 +505,19 @@ func NewCommitStatus(ctx context.Context, opts NewCommitStatusOptions) error { opts.CommitStatus.Description = strings.TrimSpace(opts.CommitStatus.Description) opts.CommitStatus.Context = strings.TrimSpace(opts.CommitStatus.Context) opts.CommitStatus.TargetURL = strings.TrimSpace(opts.CommitStatus.TargetURL) + opts.CommitStatus.ContextHash = strings.TrimSpace(opts.CommitStatus.ContextHash) opts.CommitStatus.SHA = opts.SHA.String() opts.CommitStatus.CreatorID = opts.Creator.ID opts.CommitStatus.RepoID = opts.Repo.ID opts.CommitStatus.Index = idx log.Debug("NewCommitStatus[%s, %s]: %d", opts.Repo.FullName(), opts.SHA, opts.CommitStatus.Index) - opts.CommitStatus.ContextHash = hashCommitStatusContext(opts.CommitStatus.Context) + // Callers may pre-compute a ContextHash to keep entries that share a + // human-readable Context separated (e.g. two workflow files with the + // same `name:` — issue #35699). Only derive from Context when unset. + if opts.CommitStatus.ContextHash == "" { + opts.CommitStatus.ContextHash = HashCommitStatusContext(opts.CommitStatus.Context) + } // Insert new CommitStatus if err = db.Insert(ctx, opts.CommitStatus); err != nil { @@ -529,8 +535,11 @@ type SignCommitWithStatuses struct { *asymkey_model.SignCommit } -// hashCommitStatusContext hash context -func hashCommitStatusContext(context string) string { +// HashCommitStatusContext returns the sha1 hash used to dedupe commit statuses +// by Context. Callers that need to keep statuses with the same display Context +// separated (e.g. distinct workflow files sharing a `name:`) can mix extra +// disambiguating data into the input. +func HashCommitStatusContext(context string) string { return fmt.Sprintf("%x", sha1.Sum([]byte(context))) } diff --git a/services/actions/commit_status.go b/services/actions/commit_status.go index 1e60b5506a..2b8ed19f92 100644 --- a/services/actions/commit_status.go +++ b/services/actions/commit_status.go @@ -139,10 +139,24 @@ func getCommitStatusEventNameAndCommitID(run *actions_model.ActionRun) (event, c func createCommitStatus(ctx context.Context, repo *repo_model.Repository, event, commitID string, run *actions_model.ActionRun, job *actions_model.ActionRunJob) error { // TODO: store workflow name as a field in ActionRun to avoid parsing runName := path.Base(run.WorkflowID) + // fall back to the file name when the workflow has no non-blank `name:` if wfs, err := jobparser.Parse(job.WorkflowPayload); err == nil && len(wfs) > 0 { - runName = wfs[0].Name + if name := strings.TrimSpace(wfs[0].Name); name != "" { + runName = name + } } ctxName := strings.TrimSpace(fmt.Sprintf("%s / %s (%s)", runName, job.Name, event)) // git_model.NewCommitStatus also trims spaces + // Mix the workflow file path into the hash so two workflow files that + // share the same `name:` and job name produce distinct commit statuses + // even though they render identically — matching GitHub's behavior + // (issue #35699). + ctxHash := git_model.HashCommitStatusContext(ctxName + "\x00" + run.WorkflowID) + // Pre-fix rows were hashed from Context alone. If a pre-existing row with + // the legacy hash is still the "latest" for this SHA, reuse that hash so + // the new row supersedes it; otherwise the old pending status would stay + // stuck forever (it lives in its own dedupe group). Only relevant for + // in-flight workflows at upgrade time. + legacyHash := git_model.HashCommitStatusContext(ctxName) state := toCommitStatus(job.Status) targetURL := fmt.Sprintf("%s/jobs/%d", run.Link(), job.ID) description := toCommitStatusDescription(job) @@ -152,7 +166,13 @@ func createCommitStatus(ctx context.Context, repo *repo_model.Repository, event, return fmt.Errorf("GetLatestCommitStatus: %w", err) } for _, v := range statuses { - if v.Context == ctxName { + if v.ContextHash == legacyHash && v.Context == ctxName { + ctxHash = legacyHash + break + } + } + for _, v := range statuses { + if v.ContextHash == ctxHash { if v.State == state && v.TargetURL == targetURL && v.Description == description { return nil } @@ -166,6 +186,7 @@ func createCommitStatus(ctx context.Context, repo *repo_model.Repository, event, TargetURL: targetURL, Description: description, Context: ctxName, + ContextHash: ctxHash, State: state, CreatorID: creator.ID, } diff --git a/services/actions/commit_status_test.go b/services/actions/commit_status_test.go index c6bcfddf18..e20dcf8511 100644 --- a/services/actions/commit_status_test.go +++ b/services/actions/commit_status_test.go @@ -11,8 +11,10 @@ import ( git_model "gitea.dev/models/git" repo_model "gitea.dev/models/repo" "gitea.dev/models/unittest" + user_model "gitea.dev/models/user" actions_module "gitea.dev/modules/actions" "gitea.dev/modules/commitstatus" + "gitea.dev/modules/git" "gitea.dev/modules/gitrepo" "gitea.dev/modules/timeutil" @@ -146,6 +148,158 @@ func TestGetCommitActionsStatusMap(t *testing.T) { assert.Empty(t, nilInfo.IconStatus(statuses[0])) } +// TestCreateCommitStatus_DistinctWorkflowFilesSameName covers issue #35699: +// two workflow files with the same `name:` and same job name must produce +// two distinct commit statuses, not be deduplicated into one. +func TestCreateCommitStatus_DistinctWorkflowFilesSameName(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 4}) + branch := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo.ID, Name: repo.DefaultBranch}) + + payload := []byte(` +name: test-run +on: pull_request +jobs: + my-test: + runs-on: ubuntu-latest + steps: + - run: echo hi +`) + + for _, spec := range []struct { + workflowID string + runID, jobID int64 + }{ + {"workflow1.yaml", 99101, 99201}, + {"workflow2.yaml", 99102, 99202}, + } { + run := &actions_model.ActionRun{ + ID: spec.runID, Index: spec.runID, RepoID: repo.ID, Repo: repo, OwnerID: repo.OwnerID, TriggerUserID: repo.OwnerID, + WorkflowID: spec.workflowID, CommitSHA: branch.CommitID, + } + require.NoError(t, db.Insert(t.Context(), run)) + job := &actions_model.ActionRunJob{ + ID: spec.jobID, RunID: run.ID, RepoID: repo.ID, OwnerID: repo.OwnerID, + Name: "my-test", Status: actions_model.StatusWaiting, + WorkflowPayload: payload, + } + require.NoError(t, db.Insert(t.Context(), job)) + require.NoError(t, createCommitStatus(t.Context(), repo, "pull_request", branch.CommitID, run, job)) + } + + statuses, err := git_model.GetLatestCommitStatus(t.Context(), repo.ID, branch.CommitID, db.ListOptionsAll) + require.NoError(t, err) + + // Both workflow files should produce a row even though the display + // Context is identical — matching GitHub's behavior. + hashes := map[string]struct{}{} + targets := map[string]struct{}{} + for _, st := range statuses { + hashes[st.ContextHash] = struct{}{} + targets[st.TargetURL] = struct{}{} + assert.Equal(t, "test-run / my-test (pull_request)", st.Context) + } + assert.Len(t, hashes, 2, "expected distinct ContextHash per workflow file") + assert.Len(t, targets, 2, "expected distinct TargetURL per workflow file") +} + +// TestCreateCommitStatus_LegacyHashRecovery covers the upgrade path: a pending +// status created before the fix (hashed from Context alone) must still be +// superseded by a follow-up event, instead of being orphaned in its own dedupe +// group while a new row accumulates under the new hash. +func TestCreateCommitStatus_LegacyHashRecovery(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 4}) + branch := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo.ID, Name: repo.DefaultBranch}) + + ctxName := "legacy.yaml / my-job (push)" + legacyHash := git_model.HashCommitStatusContext(ctxName) + sha, err := git.NewIDFromString(branch.CommitID) + require.NoError(t, err) + creator := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) + require.NoError(t, git_model.NewCommitStatus(t.Context(), git_model.NewCommitStatusOptions{ + Repo: repo, + Creator: creator, + SHA: sha, + CommitStatus: &git_model.CommitStatus{ + State: commitstatus.CommitStatusPending, + Context: ctxName, + ContextHash: legacyHash, + TargetURL: "https://example.invalid/legacy", + Description: "Waiting to run", + }, + })) + + run := &actions_model.ActionRun{ + ID: 99301, Index: 99301, RepoID: repo.ID, Repo: repo, OwnerID: repo.OwnerID, TriggerUserID: repo.OwnerID, + WorkflowID: "legacy.yaml", CommitSHA: branch.CommitID, + } + require.NoError(t, db.Insert(t.Context(), run)) + job := &actions_model.ActionRunJob{ + ID: 99302, RunID: run.ID, RepoID: repo.ID, OwnerID: repo.OwnerID, + Name: "my-job", Status: actions_model.StatusSuccess, + } + require.NoError(t, db.Insert(t.Context(), job)) + require.NoError(t, createCommitStatus(t.Context(), repo, "push", branch.CommitID, run, job)) + + latest, err := git_model.GetLatestCommitStatus(t.Context(), repo.ID, branch.CommitID, db.ListOptionsAll) + require.NoError(t, err) + // The new row must reuse the legacy hash so GetLatestCommitStatus returns + // only one entry for this Context — the success, not the orphaned pending. + matches := 0 + for _, s := range latest { + if s.Context == ctxName { + matches++ + assert.Equal(t, legacyHash, s.ContextHash) + assert.Equal(t, commitstatus.CommitStatusSuccess, s.State) + } + } + assert.Equal(t, 1, matches) +} + +// TestCreateCommitStatus_UnnamedWorkflowUsesFileName: a workflow with no +// non-blank `name:` uses the file name in the Context, not an empty +// "/ job (event)" — covers both an omitted and a whitespace-only name. +func TestCreateCommitStatus_UnnamedWorkflowUsesFileName(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 4}) + branch := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo.ID, Name: repo.DefaultBranch}) + + for _, tc := range []struct { + workflowID string + runID, jobID int64 + payload string + }{ + {"unnamed.yaml", 99401, 99411, "on: push\n"}, + {"blank.yaml", 99402, 99412, "name: \" \"\non: push\n"}, + } { + run := &actions_model.ActionRun{ + ID: tc.runID, Index: tc.runID, RepoID: repo.ID, Repo: repo, OwnerID: repo.OwnerID, TriggerUserID: repo.OwnerID, + WorkflowID: tc.workflowID, CommitSHA: branch.CommitID, + } + require.NoError(t, db.Insert(t.Context(), run)) + job := &actions_model.ActionRunJob{ + ID: tc.jobID, RunID: run.ID, RepoID: repo.ID, OwnerID: repo.OwnerID, + Name: "my-test", Status: actions_model.StatusWaiting, + WorkflowPayload: []byte(tc.payload + `jobs: + my-test: + runs-on: ubuntu-latest + steps: + - run: echo hi +`), + } + require.NoError(t, db.Insert(t.Context(), job)) + require.NoError(t, createCommitStatus(t.Context(), repo, "push", branch.CommitID, run, job)) + + statuses := findCommitStatusesForContext(t, repo.ID, branch.CommitID, tc.workflowID+" / my-test (push)") + require.Len(t, statuses, 1) + assert.Equal(t, commitstatus.CommitStatusPending, statuses[0].State) + } +} + func findCommitStatusesForContext(t *testing.T, repoID int64, sha, context string) []*git_model.CommitStatus { t.Helper() diff --git a/tests/integration/actions_trigger_test.go b/tests/integration/actions_trigger_test.go index cd87bc8b1a..d222aac362 100644 --- a/tests/integration/actions_trigger_test.go +++ b/tests/integration/actions_trigger_test.go @@ -638,7 +638,7 @@ jobs: return false } if latestCommitStatuses[0].State == commitstatus.CommitStatusPending { - insertFakeStatus(t, repo, sha, latestCommitStatuses[0].TargetURL, latestCommitStatuses[0].Context) + insertFakeStatus(t, repo, sha, latestCommitStatuses[0]) return true } return false @@ -680,14 +680,18 @@ func checkCommitStatusAndInsertFakeStatus(t *testing.T, repo *repo_model.Reposit assert.Len(t, latestCommitStatuses, 1) assert.Equal(t, commitstatus.CommitStatusPending, latestCommitStatuses[0].State) - insertFakeStatus(t, repo, sha, latestCommitStatuses[0].TargetURL, latestCommitStatuses[0].Context) + insertFakeStatus(t, repo, sha, latestCommitStatuses[0]) } -func insertFakeStatus(t *testing.T, repo *repo_model.Repository, sha, targetURL, context string) { +// insertFakeStatus inserts a success status that lands in the same dedupe +// group as `prev` — the actions runner mixes the workflow file path into +// ContextHash, so we must reuse it (rather than recomputing from Context). +func insertFakeStatus(t *testing.T, repo *repo_model.Repository, sha string, prev *git_model.CommitStatus) { err := commitstatus_service.CreateCommitStatus(t.Context(), repo, user_model.NewActionsUser(), sha, &git_model.CommitStatus{ - State: commitstatus.CommitStatusSuccess, - TargetURL: targetURL, - Context: context, + State: commitstatus.CommitStatusSuccess, + TargetURL: prev.TargetURL, + Context: prev.Context, + ContextHash: prev.ContextHash, }) assert.NoError(t, err) } @@ -822,7 +826,7 @@ jobs: return false } if latestCommitStatuses[0].State == commitstatus.CommitStatusPending { - insertFakeStatus(t, repo, sha, latestCommitStatuses[0].TargetURL, latestCommitStatuses[0].Context) + insertFakeStatus(t, repo, sha, latestCommitStatuses[0]) return true } return false