diff --git a/internal/azure/azure.go b/internal/azure/azure.go index a7d57c028..b83cf56ab 100644 --- a/internal/azure/azure.go +++ b/internal/azure/azure.go @@ -167,21 +167,24 @@ func (c *AzureConfig) GetPullRequestCommits(pr git.GitPullRequest) ([]types.Comm } for _, commit := range prCommitsResponse.Value { - commits = append(commits, types.Commit{ - SHA: *commit.CommitId, - Message: *commit.Comment, - Author: *commit.Author.Name, - Timestamp: commit.Committer.Date.Time.Unix(), - URL: *commit.Url, - Branch: *pr.SourceRefName, - // TODO(server#5479): AuthorUsername is sourced from the committer; should be the author. - AuthorUsername: *commit.Committer.Name, - }) + commits = append(commits, commitFromAzureCommit(commit, *pr.SourceRefName)) } return commits, nil } +// commitFromAzureCommit maps an Azure DevOps API commit to a types.Commit. +func commitFromAzureCommit(commit git.GitCommitRef, branch string) types.Commit { + return types.Commit{ + SHA: *commit.CommitId, + Message: *commit.Comment, + Author: fmt.Sprintf("%s <%s>", *commit.Author.Name, *commit.Author.Email), + Timestamp: commit.Author.Date.Time.Unix(), + URL: *commit.Url, + Branch: branch, + } +} + // PullRequestsForCommit returns a list of pull requests for a specific commit func (c *AzureConfig) PullRequestsForCommit(commit string) ([]git.GitPullRequest, error) { ctx := context.Background() diff --git a/internal/azure/commit_mapping_test.go b/internal/azure/commit_mapping_test.go new file mode 100644 index 000000000..5343334cd --- /dev/null +++ b/internal/azure/commit_mapping_test.go @@ -0,0 +1,54 @@ +package azure + +import ( + "testing" + "time" + + "github.com/microsoft/azure-devops-go-api/azuredevops" + "github.com/microsoft/azure-devops-go-api/azuredevops/git" + "github.com/stretchr/testify/require" +) + +func azStr(s string) *string { return &s } + +func azTestCommit() git.GitCommitRef { + authorDate := azuredevops.Time{Time: time.Unix(1772630000, 0)} + committerDate := azuredevops.Time{Time: time.Unix(1772635812, 0)} + return git.GitCommitRef{ + CommitId: azStr("abc1230000000000000000000000000000000000"), + Comment: azStr("Apply suggestions from code review"), + Url: azStr("https://dev.azure.com/kosli/_git/x/commit/abc123"), + Author: &git.GitUserDate{ + Name: azStr("Steve Tooke"), + Email: azStr("tooky@kosli.com"), + Date: &authorDate, + }, + Committer: &git.GitUserDate{ + Name: azStr("GitHub"), + Email: azStr("noreply@github.com"), + Date: &committerDate, + }, + } +} + +// TestCommitFromAzureCommit_RecordsAuthorIdentity is a regression test for +// server#5479. Azure recorded the author display name only, and populated +// author_username from the committer. Record the author as "Name ", +// and drop author_username (Azure commits carry no login). +func TestCommitFromAzureCommit_RecordsAuthorIdentity(t *testing.T) { + c := commitFromAzureCommit(azTestCommit(), "my-branch") + + require.Equal(t, "Steve Tooke ", c.Author, + "author must be name of the git author") + require.Empty(t, c.AuthorUsername, + "Azure commits carry no login; author_username must be omitted, not the committer name") +} + +// TestCommitFromAzureCommit_UsesAuthorDate is a regression test for server#5479: +// the timestamp must match the recorded author identity. +func TestCommitFromAzureCommit_UsesAuthorDate(t *testing.T) { + c := commitFromAzureCommit(azTestCommit(), "my-branch") + + require.Equal(t, int64(1772630000), c.Timestamp, + "timestamp must be the author date, not the committer date") +} diff --git a/internal/github/build_pr_evidence_test.go b/internal/github/build_pr_evidence_test.go index d5f01608d..e27d06d2f 100644 --- a/internal/github/build_pr_evidence_test.go +++ b/internal/github/build_pr_evidence_test.go @@ -2,6 +2,7 @@ package github import ( "testing" + "time" "github.com/shurcooL/graphql" "github.com/stretchr/testify/require" @@ -53,3 +54,57 @@ func TestBuildPREvidence_RecordsAuthorNotCommitter(t *testing.T) { require.Equal(t, "tooky", c.AuthorUsername, "author_username must be the author's GitHub login, not the committer's (absent) one") } + +// TestBuildPREvidence_UsesAuthoredDate is a regression test for server#5479. +// Now that the recorded identity is the author, the timestamp should be the +// author date too. For rebased / applied-suggestion commits the authored and +// committed dates differ. +func TestBuildPREvidence_UsesAuthoredDate(t *testing.T) { + node := graphqlCommitNode{} + node.Commit.Oid = "0e723254516c841126e81f76100be57258ff1386" + node.Commit.MessageHeadline = "Apply suggestions from code review" + node.Commit.AuthoredDate = "2026-03-01T10:00:00Z" + node.Commit.CommittedDate = "2026-03-01T12:00:00Z" + node.Commit.Author.Name = "Steve Tooke" + node.Commit.Author.Email = "tooky@kosli.com" + + evidence, err := buildPREvidence( + "https://github.com/kosli-dev/cli/pull/671", + "0e723254516c841126e81f76100be57258ff1386", + "MERGED", "tooky", "2026-03-01T09:00:00Z", "", + "Introduce kosli evaluate", "introduce-kosli-evaluate", + []graphqlCommitNode{node}, nil, + ) + require.NoError(t, err) + require.Len(t, evidence.Commits, 1) + + wantAuthored, _ := time.Parse(time.RFC3339, "2026-03-01T10:00:00Z") + require.Equal(t, wantAuthored.Unix(), evidence.Commits[0].Timestamp, + "timestamp must be the author date, not the committer date") +} + +// TestBuildPREvidence_FallsBackToCommittedDate ensures the timestamp falls back +// to the committed date when the GraphQL response omits the authored date. +func TestBuildPREvidence_FallsBackToCommittedDate(t *testing.T) { + node := graphqlCommitNode{} + node.Commit.Oid = "0e723254516c841126e81f76100be57258ff1386" + node.Commit.MessageHeadline = "msg" + node.Commit.AuthoredDate = "" + node.Commit.CommittedDate = "2026-03-01T12:00:00Z" + node.Commit.Author.Name = "Steve Tooke" + node.Commit.Author.Email = "tooky@kosli.com" + + evidence, err := buildPREvidence( + "https://github.com/kosli-dev/cli/pull/671", + "0e723254516c841126e81f76100be57258ff1386", + "MERGED", "tooky", "2026-03-01T09:00:00Z", "", + "title", "branch", + []graphqlCommitNode{node}, nil, + ) + require.NoError(t, err) + require.Len(t, evidence.Commits, 1) + + wantCommitted, _ := time.Parse(time.RFC3339, "2026-03-01T12:00:00Z") + require.Equal(t, wantCommitted.Unix(), evidence.Commits[0].Timestamp, + "timestamp must fall back to the committed date when authored date is absent") +} diff --git a/internal/github/github.go b/internal/github/github.go index 886a781a0..cb9b4fac8 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -263,6 +263,7 @@ type graphqlCommitNode struct { Oid graphql.String MessageHeadline graphql.String CommittedDate graphql.String + AuthoredDate graphql.String URL graphql.String Committer struct { Name graphql.String @@ -326,9 +327,13 @@ func buildPREvidence( } for _, n := range commitNodes { - // TODO(server#5479): timestamp uses the committer date; consider authoredDate for - // author-consistency (cross-provider: also Azure commit.Committer.Date, GitLab CreatedAt). - timestamp, err := time.Parse(time.RFC3339, string(n.Commit.CommittedDate)) + // Use the author date to match the recorded author identity; fall back to + // the committed date if the API omits it (server#5479). + dateStr := string(n.Commit.AuthoredDate) + if dateStr == "" { + dateStr = string(n.Commit.CommittedDate) + } + timestamp, err := time.Parse(time.RFC3339, dateStr) if err != nil { return nil, err } diff --git a/internal/gitlab/commit_mapping_test.go b/internal/gitlab/commit_mapping_test.go new file mode 100644 index 000000000..15554426a --- /dev/null +++ b/internal/gitlab/commit_mapping_test.go @@ -0,0 +1,72 @@ +package gitlab + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" + gitlab "gitlab.com/gitlab-org/api/client-go" +) + +// TestCommitFromGitlabCommit_RecordsAuthorNotCommitter is a regression test for +// server#5479. PR commit attestations were recording the git committer in the +// author field. They must record the git author. +func TestCommitFromGitlabCommit_RecordsAuthorNotCommitter(t *testing.T) { + created := time.Unix(1772635812, 0) + commit := &gitlab.Commit{ + ID: "abc1230000000000000000000000000000000000", + Message: "Apply suggestions from code review", + AuthorName: "Steve Tooke", + AuthorEmail: "tooky@kosli.com", + CommitterName: "GitHub", + CommitterEmail: "noreply@github.com", + CreatedAt: &created, + WebURL: "https://gitlab.com/kosli/x/-/commit/abc123", + } + + c := commitFromGitlabCommit(commit, "my-branch") + + require.Equal(t, "Steve Tooke ", c.Author, + "author must be the git author, not the committer") +} + +// TestCommitFromGitlabCommit_UsesAuthoredDate is a regression test for +// server#5479: the timestamp must match the recorded author identity. +func TestCommitFromGitlabCommit_UsesAuthoredDate(t *testing.T) { + authored := time.Unix(1772630000, 0) + created := time.Unix(1772635812, 0) + commit := &gitlab.Commit{ + ID: "abc1230000000000000000000000000000000000", + Message: "msg", + AuthorName: "Steve Tooke", + AuthorEmail: "tooky@kosli.com", + AuthoredDate: &authored, + CreatedAt: &created, + WebURL: "https://gitlab.com/kosli/x/-/commit/abc123", + } + + c := commitFromGitlabCommit(commit, "my-branch") + + require.Equal(t, int64(1772630000), c.Timestamp, + "timestamp must be the authored date, not created_at") +} + +// TestCommitFromGitlabCommit_FallsBackToCreatedAt ensures the timestamp falls +// back to created_at when the API omits the authored date. +func TestCommitFromGitlabCommit_FallsBackToCreatedAt(t *testing.T) { + created := time.Unix(1772635812, 0) + commit := &gitlab.Commit{ + ID: "abc1230000000000000000000000000000000000", + Message: "msg", + AuthorName: "Steve Tooke", + AuthorEmail: "tooky@kosli.com", + AuthoredDate: nil, + CreatedAt: &created, + WebURL: "https://gitlab.com/kosli/x/-/commit/abc123", + } + + c := commitFromGitlabCommit(commit, "my-branch") + + require.Equal(t, int64(1772635812), c.Timestamp, + "timestamp must fall back to created_at when authored date is absent") +} diff --git a/internal/gitlab/gitlab.go b/internal/gitlab/gitlab.go index 281e80acd..bb901ac67 100644 --- a/internal/gitlab/gitlab.go +++ b/internal/gitlab/gitlab.go @@ -174,15 +174,25 @@ func (c *GitlabConfig) GetMergeRequestCommits(mr *gitlab.BasicMergeRequest) ([]t return commits, err } for _, commit := range glCommits { - commits = append(commits, types.Commit{ - SHA: commit.ID, - Message: commit.Message, - // TODO(server#5479): author is sourced from the committer; switch to AuthorName/AuthorEmail. - Author: fmt.Sprintf("%s <%s>", commit.CommitterName, commit.CommitterEmail), - Timestamp: commit.CreatedAt.Unix(), - Branch: mr.SourceBranch, - URL: commit.WebURL, - }) + commits = append(commits, commitFromGitlabCommit(commit, mr.SourceBranch)) } return commits, nil } + +// commitFromGitlabCommit maps a GitLab API commit to a types.Commit. +func commitFromGitlabCommit(commit *gitlab.Commit, branch string) types.Commit { + // Use the authored date to match the recorded author identity; fall back to + // created_at if the API omits it (server#5479). + timestamp := commit.CreatedAt.Unix() + if commit.AuthoredDate != nil { + timestamp = commit.AuthoredDate.Unix() + } + return types.Commit{ + SHA: commit.ID, + Message: commit.Message, + Author: fmt.Sprintf("%s <%s>", commit.AuthorName, commit.AuthorEmail), + Timestamp: timestamp, + Branch: branch, + URL: commit.WebURL, + } +}