fix(github): serialise empty PR author instead of omitting it

[vidhu-balad]

Summary

• When a PR creator's GitHub account is deleted, the GraphQL API returns null for the Author node, resulting in author = "" in Go
• PREvidence.Author has omitempty, so an empty string is omitted from the JSON payload entirely
• The server's FoundPullRequestV2.author is a required field — missing it causes validation to fail with "input should be a string extra inputs are not permitted"
• Fix: guard in buildPREvidence substitutes "unknown" when author == "", covering both PREvidenceForCommitV2 and PREvidenceByPRNumber call sites

Test plan

• TestBuildPREvidence_EmptyAuthorFallsBackToUnknown added and passes
• All internal/github tests pass (go test ./internal/github/...)
• go vet ./internal/github/... is clean

🤖 Generated with Claude Code

[claude]

Claude finished @vidhu-balad's task in 1m 56s —— View job

---

Review: PR #1020 — empty PR author serialisation

• Read changed files & production code
• Run static analysis (go build, go vet)
• Verify the earlier review concern (JSON-marshal guard) is addressed
• Assess the reviewer-flagged PRApprovals.Username follow-up
• Post review summary

---

Static analysis ✅ go build ./... and go vet ./internal/... ./cmd/kosli/... both clean. (Full test run needs the local Kosli server, so I verified the changed test by inspection rather than execution.)

✅ The main review concern is now resolved

The repeated review note — that require.Equal(t, "", evidence.Author, …) is trivially true and wouldn't catch someone re-adding omitempty — has been addressed by commit ec85bb4c. internal/github/build_pr_evidence_test.go:177-180 now marshals the evidence and asserts:

require.Contains(t, string(b), `"author":""`,
    "empty author must be serialised, not omitted")

This is the real guard: with omitempty removed (types.go:8) an empty author serialises as "author":""; if the tag were re-added, json.Marshal would drop the key and this assertion would fail. Correct, minimal, and now properly pinned. 👍

Observations (non-blocking)

1. PR title/description are now stale. The title still says "fall back to unknown" and the body describes substituting "unknown", but the implementation evolved (commit c1fe9ac2) to send the empty string "" verbatim instead. Worth updating the title/description before merge so the history reflects the actual approach — sending "" is the better choice here since it preserves the true state rather than inventing a fake login. Also relevant to @mbevc1's note that GitHub attributes deleted accounts to @ghost; the empty-string approach sidesteps guessing a substitute identity, which seems reasonable, but if you'd prefer wire-level consistency with GitHub's own convention, @ghost would be the alternative — a product decision, not a correctness one.

2. Reviewer/approver empty-author (raised by @mbevc1 and the earlier claude review): now consistent, worth a follow-up test. PRApprovals.Username (types.go:18) has no omitempty, and buildPREvidence builds it from string(r.Author.Login) (github.go:371) with no guard. A deleted reviewer account therefore already serialises "username":"" rather than dropping the field — so it does not hit the "missing required field" error this PR fixes, and its behaviour is now consistent with the author fix (both send ""). No code change needed here, but there's no test covering the empty-approver-login path — a small regression test would lock in that consistency. Out of scope for this PR; a follow-up issue is the right home.

3. Fixture updates look good. Fleshing out the FakeGitHubClient PR fixtures to the full V2 shape (author, title, commits, refs) in assertPRGithub_test.go / attestPRGithub_test.go makes those suites exercise a realistic payload rather than a bare stub — nice hardening.

Verdict

The fix is correct and minimal, static checks pass, and the test now genuinely guards against the omitempty regression. The only actionable item is cosmetic: refresh the PR title/description to match the "send empty string" approach that actually shipped. The approver-path test is a reasonable follow-up, not a blocker.

[mbevc1]

Not a blocker and just a note GitHub officially attributes content from deleted accounts to the reserved @ghost user (REST API returns the ghost user object; the web UI shows @ghost).

[mbevc1]

Another note; same bug class exists for review authors (approvers). In buildPREvidence, approvers are built from r.Author.Login with no guard, and PRApprovals.Username has no omitempty — so a deleted reviewer account produces "username": "" in the payload.

[claude]

This test doesn't guard the actual fix. The fix lives in types.go (dropping omitempty from Author). But buildPREvidence assigns Author: author verbatim, so require.Equal(t, "", evidence.Author, ...) passes whether or not omitempty is present — the assertion is trivially true and would stay green if someone re-added the tag.

To actually pin the behavior, assert on the marshaled JSON so the author key is proven present when empty:

b, err := json.Marshal(evidence)
require.NoError(t, err)
require.Contains(t, string(b), `"author":""`,
    "empty author must be serialised, not omitted")

Fix this →

[claude]

Good, targeted fix — removing omitempty is the minimal change that makes an empty author serialise instead of vanish.

Follow-up on @mbevc1's note about the same bug class for reviewers: PRApprovals.Username (line 18, built from r.Author.Login at github.go:371) has no omitempty, so a deleted reviewer account already serialises "username": "" rather than dropping the field — it won't hit the identical "missing required field" error, but it does send an empty username. If the server's approvers schema also rejects empty strings, that's a latent failure with the same root cause. Out of scope here, but worth a follow-up issue.

[claude]

This assertion still doesn't guard the actual fix (raised in the earlier review, not yet addressed).

The fix is purely the JSON struct-tag change in types.go (dropping omitempty). But buildPREvidence assigns Author: author verbatim (github.go:316), so require.Equal(t, "", evidence.Author, …) is trivially true regardless of the struct tag — it stays green even if someone re-adds omitempty, which would re-introduce the exact bug this PR fixes.

To pin the behavior, assert on the marshaled JSON so the author key is proven present when empty:

require.NoError(t, err)
require.Equal(t, "", evidence.Author,
    "empty PR author login must be preserved")

b, err := json.Marshal(evidence)
require.NoError(t, err)
require.Contains(t, string(b), `"author":""`,
    "empty author must be serialised, not omitted")

(requires adding "encoding/json" to the import block.)

Fix this →

[claude]

This assertion doesn't guard the fix it's named for. The fix is entirely the struct-tag change in types.go (dropping omitempty). But buildPREvidence assigns Author: author verbatim (github.go:316), so require.Equal(t, "", evidence.Author, …) is true regardless of the tag — the test stays green even if someone re-adds omitempty, re-introducing the exact bug this PR fixes.

Assert on the marshaled JSON so the author key is proven present when empty:

Suggested change

	require.NoError(t, err)
	require.Equal(t, "", evidence.Author,
	"empty PR author login must be preserved so it is serialised as an empty string, not omitted")
	require.NoError(t, err)
	require.Equal(t, "", evidence.Author,
	"empty PR author login must be preserved so it is serialised as an empty string, not omitted")
	b, err := json.Marshal(evidence)
	require.NoError(t, err)
	require.Contains(t, string(b), `"author":""`,
	"empty author must be serialised, not omitted (guards against re-adding omitempty)")

Requires adding "encoding/json" to the import block (line 3–9).