fix(attest): always serialise commits in pull request attestations

[dangrondahl]

Fixes #1081.

GitLab stopped returning commits for merge requests whose diff predates ~2025-11-26. GetMergeRequestCommits passed the empty list through as an empty slice with no error, omitempty then deleted commits from the JSON, and the API rejected a payload matching neither FoundPullRequestV1 (which forbids the extended fields) nor FoundPullRequestV2 (which requires commits).

fix(attest): always serialise commits

Drop omitempty from PREvidence.Commits and normalise nil to [] in MarshalJSON. A nil slice would otherwise serialise as null, which fails validation just as surely as a missing field, so removing the tag alone is not enough.

commits: [] validates against FoundPullRequestV2, and the server's set_relevant_commits validator handles an empty list — the merge commit is still recorded.

Safe for the V1 shape: PREvidenceForCommitV1 is only reached by assert pullrequest bitbucket and assert pullrequest azure, which count merge requests and post nothing. GitHub's hybrid fallback builds V2-shaped evidence via PREvidenceByPRNumber. The only serialised shape is V2.

test(attest): run the gitlab suite against a fake

The suite pinned merkely-gitlab-demo !1, merged 2024-10-10 — a fixture that can never return commits again. Add NewGitlabRetrieverFunc and FakeGitlabClient, mirroring the existing GitHub seam and FakeGitHubClient, and point the suite at a fake seeded with a merge request that has a commit.

Without this the suite would pass while asserting a zero-commit attestation, losing the coverage the fix is meant to protect. The git repo is still cloned for real, so commit resolution stays honest; only the merge request API is faked.

Verification

go build ./..., go vet, make lint (0 issues), and internal/types + internal/gitlab all pass. TestPREvidenceAlwaysSerialisesCommits was confirmed red before the fix and green after.

TestAttestGitlabPRCommandTestSuite has not been run. It needs the local Kosli server, and CloneGitRepo additionally fails on macOS with open .../.git: is a directory — a /var symlink quirk reproduced on an unmodified tree, so pre-existing and unrelated. CI is the first real signal for the fake wiring.

Note the suite is no longer gated on KOSLI_GITLAB_TOKEN, so it now runs rather than skips when that secret is absent.

Not included

• Warning when a provider returns zero commits — GitlabConfig has no logger; worth doing, but separate.
• Normalising Approvers nil to [] — cosmetic, the API accepts null.
• Moving the live-API assertions to a gitlab_contract_test.go behind make test_contract_gitlab. Blocked on whether GitLab's retention is a rolling window or a one-off deletion: if rolling, a contract test must select a recent merge request dynamically rather than pin a SHA, or it breaks the same way in ~9 months. Re-probing gitlab-org/gitlab !212000 and !214582 in a week answers it (see bug: GitLab PR attestation sends an invalid payload when the MR has no commits (CI red) #1081).
• assert pullrequest gitlab still hits live GitLab with the same 2024 fixture. It passes today because assert never needs commits, but it carries the same fragility.

[claude]

Claude finished @dangrondahl's task in 3m 25s —— View job

---

Claude PR Review

• Static analysis (go build, go vet, go mod tidy)
• Read changed files & traced serialisation paths
• Reviewed code quality, edge cases, bugs, tests
• Posted review feedback

---

Static analysis

Check	Result
go build ./...	✅ pass
go vet (changed pkgs)	✅ pass
go mod tidy + diff	✅ clean (no dep changes)

Verdict

This is a well-reasoned, correctly-scoped fix. The root-cause analysis in the description is accurate, and I independently verified the two claims the fix rests on:

• Only V2 is serialised. All four attest pullrequest commands post via pullrequest.go:45 → PREvidenceForCommitHybrid, and every provider's Hybrid delegates to V2 (gitlab.go:81, azure.go:79, bitbucket.go:51, github.go:231). So commits: [] only ever reaches the FoundPullRequestV2 shape that requires it. ✅
• V1/assert posts nothing. assertPRGithub.go:62 (and the Azure/Bitbucket equivalents) only read len(pullRequestsEvidence) — no marshalling — so the "V1 forbids extended fields" schema is never sent. ✅

What's good 👍

• MarshalJSON recursion guard — the type prEvidence PREvidence alias to shed the method set is the correct idiom, and it mutates a local copy so the caller's slice is untouched. The comment explaining why nil→[] (not just dropping omitempty) is exactly the kind of comment that earns its keep.
• Retriever seam mirrors the existing pattern. NewGitlabRetrieverFunc + ResetGitlabRetrieverFunc + FakeGitlabClient faithfully follow the established GitHub seam (NewGithubRetrieverFunc / FakeGitHubClient), so it reads as idiomatic to this codebase rather than a new invention.
• The fake already normalises nil → [] (fake_gitlab.go), directly addressing the prior-run review note — and fake_gitlab_test.go even pins the "seeded-nil" case so the fake can't silently drift from the real client's shape. Nice.
• Honest test seam — faking only the merge-request API while still cloning the repo for real keeps commit resolution genuine. Good instinct, and the inline comment says so.

Observations (non-blocking)

1. Blast radius of the shared MarshalJSON (inline note on internal/types/types.go): because it's on the shared PREvidence type, Azure and Bitbucket V2 payloads now also gain commits: []. This is more correct, not a regression — but it's currently untested for those two providers. A one-line marshal test would lock it in.
2. RunE passes o.repoName, not gitlabFlagsValues.Repository — correct and intentional: addAttestationFlags registers --repository first, so addGitlabFlags's conditional binding (flags.go:79) is skipped and gitlabFlagsValues.Repository stays empty. Worth being aware of, no change needed.
3. NewGitlabRetrieverFunc is a mutable package global — a test-only seam, safe here because testify suites run sequentially within the package and TearDownTest restores it. Consistent with the GitHub sibling.

On the "Not included" list

The deferrals are all reasonable, and I agree with keeping them separate. The most valuable follow-up is the zero-commits warning (blocked only on GitlabConfig gaining a logger) — silently attesting zero commits is exactly the failure mode that hid this bug for months. The contract-test question (rolling window vs one-off deletion) is the right thing to resolve before investing in gitlab_contract_test.go.

Nothing blocking. CI is indeed the first real signal for the fake wiring and the local-server suite, as the description notes.
• fix-gitlab-pr-empty-commits