diff --git a/cmd/kosli/attestPRGitlab.go b/cmd/kosli/attestPRGitlab.go index a752638e4..cbbb1476f 100644 --- a/cmd/kosli/attestPRGitlab.go +++ b/cmd/kosli/attestPRGitlab.go @@ -100,8 +100,8 @@ func newAttestGitlabPRCmd(out io.Writer) *cobra.Command { payload: PRAttestationPayload{ CommonAttestationPayload: &CommonAttestationPayload{}, }, - retriever: new(gitlabUtils.GitlabConfig), } + gitlabFlagsValues := new(gitlabUtils.GitlabConfig) cmd := &cobra.Command{ // Args: cobra.MaximumNArgs(1), // See CustomMaximumNArgs() below Use: "gitlab [IMAGE-NAME | FILE-PATH | DIR-PATH]", @@ -146,14 +146,15 @@ func newAttestGitlabPRCmd(out io.Writer) *cobra.Command { // GitlabConfig.Repository is the short project name (CI_PROJECT_NAME); // combined with Org (CI_PROJECT_NAMESPACE) it forms the API ProjectID. // This is separate from repo_info.name, which uses the full CI_PROJECT_PATH. - o.getRetriever().(*gitlabUtils.GitlabConfig).Repository = o.repoName + o.retriever = gitlabUtils.NewGitlabRetrieverFunc(gitlabFlagsValues.Token, + gitlabFlagsValues.BaseURL, gitlabFlagsValues.Org, o.repoName) return o.run(args) }, } ci := WhichCI() addAttestationFlags(cmd, o.CommonAttestationOptions, o.payload.CommonAttestationPayload, ci) - addGitlabFlags(cmd, o.getRetriever().(*gitlabUtils.GitlabConfig), ci) + addGitlabFlags(cmd, gitlabFlagsValues, ci) cmd.Flags().BoolVar(&o.assert, "assert", false, assertPREvidenceFlag) err := RequireFlags(cmd, []string{"flow", "trail", "name", diff --git a/cmd/kosli/attestPRGitlab_test.go b/cmd/kosli/attestPRGitlab_test.go index 1a10e1798..b14d3269d 100644 --- a/cmd/kosli/attestPRGitlab_test.go +++ b/cmd/kosli/attestPRGitlab_test.go @@ -5,7 +5,9 @@ import ( "os" "testing" + gitlabUtils "github.com/kosli-dev/cli/internal/gitlab" "github.com/kosli-dev/cli/internal/testHelpers" + "github.com/kosli-dev/cli/internal/types" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" ) @@ -25,8 +27,6 @@ type AttestGitlabPRCommandTestSuite struct { } func (suite *AttestGitlabPRCommandTestSuite) SetupTest() { - testHelpers.SkipIfEnvVarUnset(suite.T(), []string{"KOSLI_GITLAB_TOKEN"}) - suite.flowName = "attest-gitlab-pr" suite.trailName = "test-123" suite.commitWithPR = "f6d2c1a288f2c400c04e8451f4fdddb1f3b4ce01" @@ -44,12 +44,46 @@ func (suite *AttestGitlabPRCommandTestSuite) SetupTest() { _, err = testHelpers.CloneGitRepo("https://gitlab.com/kosli-dev/merkely-gitlab-demo.git", suite.tmpDir) require.NoError(suite.T(), err) - suite.defaultKosliArguments = fmt.Sprintf(" --flow %s --trail %s --repo-root %s --host %s --org %s --api-token %s", suite.flowName, suite.trailName, suite.tmpDir, global.Host, global.Org, global.ApiToken) + // The merge request API is faked: GitLab stopped returning commits for + // merge requests whose diff predates ~2025-11-26, which silently broke this + // suite against the live API (#1081). The git repo above is still cloned for + // real, so commit resolution stays honest. + gitlabUtils.NewGitlabRetrieverFunc = func(token, baseURL, org, repository string) types.PRRetriever { + return &gitlabUtils.FakeGitlabClient{ + MRsByCommit: map[string][]*types.PREvidence{ + suite.commitWithPR: {{ + URL: "https://gitlab.com/kosli-dev/merkely-gitlab-demo/-/merge_requests/1", + State: "merged", + Author: "Test User (@test-user)", + Title: "Changed readme to kosli-dev", + CreatedAt: 1728562890, + MergedAt: 1728563001, + HeadRef: "update-readme", + BaseRef: "main", + MergeCommit: suite.commitWithPR, + Approvers: []any{}, + Commits: []types.Commit{{ + SHA: "77fe4082df8549385462cbed0d28610f3cb59eec", + Message: "Changed readme to kosli-dev", + Author: "Tore Martin Hagen ", + Timestamp: 1728562776, + Branch: "update-readme", + }}, + }}, + }, + } + } + + suite.defaultKosliArguments = fmt.Sprintf(" --gitlab-token fake --flow %s --trail %s --repo-root %s --host %s --org %s --api-token %s", suite.flowName, suite.trailName, suite.tmpDir, global.Host, global.Org, global.ApiToken) CreateFlowWithTemplate(suite.flowName, "testdata/valid_template.yml", suite.T()) BeginTrail(suite.trailName, suite.flowName, "", suite.T()) CreateArtifactOnTrail(suite.flowName, suite.trailName, "cli", suite.artifactFingerprint, "file1", suite.T()) } +func (suite *AttestGitlabPRCommandTestSuite) TearDownTest() { + gitlabUtils.ResetGitlabRetrieverFunc() +} + func (suite *AttestGitlabPRCommandTestSuite) TearDownSuite() { if err := os.RemoveAll(suite.tmpDir); err != nil { require.NoError(suite.T(), err, "failed to remove temp dir %s", suite.tmpDir) diff --git a/internal/gitlab/fake_gitlab.go b/internal/gitlab/fake_gitlab.go new file mode 100644 index 000000000..133685c28 --- /dev/null +++ b/internal/gitlab/fake_gitlab.go @@ -0,0 +1,49 @@ +package gitlab + +import ( + "github.com/kosli-dev/cli/internal/types" +) + +// FakeGitlabClient is an in-memory implementation of types.PRRetriever for +// testing. Seed MRsByCommit with the commits and merge request evidence you +// want returned. Set Err to simulate a network or API failure. +type FakeGitlabClient struct { + // MRsByCommit maps a commit SHA to the merge request evidence returned + // for that commit. + MRsByCommit map[string][]*types.PREvidence + // Err, if set, is returned by all calls regardless of commit. + Err error +} + +func (f *FakeGitlabClient) ProviderAndLabel() (string, string) { + return "gitlab", "merge request" +} + +// PREvidenceForCommitV2 mirrors the real client: a commit with no merge +// requests yields an empty result and no error, because GitLab's +// ListMergeRequestsByCommit returns an empty list rather than an error. +// +// The result is always non-nil. The real client builds its slice up front, and +// a nil slice would serialise as null, which the API rejects for pull_requests. +func (f *FakeGitlabClient) PREvidenceForCommitV2(commit string) ([]*types.PREvidence, error) { + if f.Err != nil { + return nil, f.Err + } + mrs := f.MRsByCommit[commit] + if mrs == nil { + return []*types.PREvidence{}, nil + } + return mrs, nil +} + +// PREvidenceForCommitV1 mirrors the real client, which serves V1 from the same +// merge request lookup as V2. +func (f *FakeGitlabClient) PREvidenceForCommitV1(commit string) ([]*types.PREvidence, error) { + return f.PREvidenceForCommitV2(commit) +} + +// PREvidenceForCommitHybrid mirrors the real client, which has no V1 fallback +// for GitLab and always serves V2. +func (f *FakeGitlabClient) PREvidenceForCommitHybrid(commit string) ([]*types.PREvidence, error) { + return f.PREvidenceForCommitV2(commit) +} diff --git a/internal/gitlab/fake_gitlab_test.go b/internal/gitlab/fake_gitlab_test.go new file mode 100644 index 000000000..f085cfab4 --- /dev/null +++ b/internal/gitlab/fake_gitlab_test.go @@ -0,0 +1,39 @@ +package gitlab + +import ( + "testing" + + "github.com/kosli-dev/cli/internal/types" + "github.com/stretchr/testify/require" +) + +// The real client builds its result slice up front, so a commit with no merge +// requests yields an empty list rather than nil. The fake must match: a nil +// slice serialises as null, and the API rejects null for pull_requests. +func TestFakeGitlabClientReturnsEmptyNotNilForUnknownCommit(t *testing.T) { + client := &FakeGitlabClient{ + MRsByCommit: map[string][]*types.PREvidence{ + "known": {{URL: "https://gitlab.com/org/repo/-/merge_requests/1"}}, + // an explicitly seeded nil must be normalised too + "seeded-nil": nil, + }, + } + + for _, retrieve := range []struct { + name string + call func(string) ([]*types.PREvidence, error) + }{ + {"V2", client.PREvidenceForCommitV2}, + {"V1", client.PREvidenceForCommitV1}, + {"Hybrid", client.PREvidenceForCommitHybrid}, + } { + for _, commit := range []string{"unknown", "seeded-nil"} { + t.Run(retrieve.name+"/"+commit, func(t *testing.T) { + mrs, err := retrieve.call(commit) + require.NoError(t, err) + require.NotNil(t, mrs, "must be an empty slice, not nil") + require.Empty(t, mrs) + }) + } + } +} diff --git a/internal/gitlab/gitlab.go b/internal/gitlab/gitlab.go index fa0d7a07f..ac7dd089e 100644 --- a/internal/gitlab/gitlab.go +++ b/internal/gitlab/gitlab.go @@ -43,6 +43,24 @@ func (c *GitlabConfig) ProviderAndLabel() (string, string) { return "gitlab", "merge request" } +// NewGitlabRetrieverFunc creates a types.PRRetriever from GitLab config +// parameters. It can be replaced in tests to inject a FakeGitlabClient. +var NewGitlabRetrieverFunc = defaultNewGitlabRetriever + +func defaultNewGitlabRetriever(token, baseURL, org, repository string) types.PRRetriever { + return &GitlabConfig{ + Token: token, + BaseURL: baseURL, + Org: org, + Repository: repository, + } +} + +// ResetGitlabRetrieverFunc restores NewGitlabRetrieverFunc to its default. +func ResetGitlabRetrieverFunc() { + NewGitlabRetrieverFunc = defaultNewGitlabRetriever +} + // This is the old implementation, it will be removed after the PR payload is enhanced for all VCS providers func (c *GitlabConfig) PREvidenceForCommitV1(commit string) ([]*types.PREvidence, error) { pullRequestsEvidence := []*types.PREvidence{} diff --git a/internal/types/types.go b/internal/types/types.go index 05a6b82ea..25db609fb 100644 --- a/internal/types/types.go +++ b/internal/types/types.go @@ -1,5 +1,7 @@ package types +import "encoding/json" + type PREvidence struct { MergeCommit string `json:"merge_commit"` URL string `json:"url"` @@ -11,7 +13,20 @@ type PREvidence struct { Title string `json:"title,omitempty"` HeadRef string `json:"head_ref,omitempty"` BaseRef string `json:"base_ref,omitempty"` - Commits []Commit `json:"commits,omitempty"` + Commits []Commit `json:"commits"` +} + +// MarshalJSON keeps "commits" in the payload even when a provider returns no +// commits. The API requires the field on FoundPullRequestV2, so omitting it — +// or sending null for a nil slice — produces a payload matching neither +// attestation schema (#1081). +func (e PREvidence) MarshalJSON() ([]byte, error) { + type prEvidence PREvidence // sheds the method set, so this does not recurse + out := prEvidence(e) + if out.Commits == nil { + out.Commits = []Commit{} + } + return json.Marshal(out) } type PRApprovals struct { diff --git a/internal/types/types_test.go b/internal/types/types_test.go new file mode 100644 index 000000000..9767f40e8 --- /dev/null +++ b/internal/types/types_test.go @@ -0,0 +1,42 @@ +package types + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/require" +) + +// The API validates pull request attestations against FoundPullRequestV2, which +// requires "commits". Dropping the field when a provider returns no commits +// produces a payload that matches neither V1 nor V2 and is rejected (#1081). +func TestPREvidenceAlwaysSerialisesCommits(t *testing.T) { + for _, tc := range []struct { + name string + evidence PREvidence + want string + }{ + { + name: "empty commits serialise as an empty array", + evidence: PREvidence{Commits: []Commit{}}, + want: `[]`, + }, + { + name: "nil commits serialise as an empty array", + evidence: PREvidence{}, + want: `[]`, + }, + } { + t.Run(tc.name, func(t *testing.T) { + payload, err := json.Marshal(tc.evidence) + require.NoError(t, err) + + var decoded map[string]json.RawMessage + require.NoError(t, json.Unmarshal(payload, &decoded)) + + raw, present := decoded["commits"] + require.True(t, present, "commits must always be present in the payload") + require.JSONEq(t, tc.want, string(raw)) + }) + } +}