Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vidhu-balad wants to merge 5 commits into
base: main
Choose a base branch
from
+61 −3
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4148eb6
fix(github): fall back to "unknown" when PR creator account is deleted
vidhu-balad Jul 15, 2026
c1fe9ac
fix(github): send empty PR author as "" instead of falling back to "u…
vidhu-balad Jul 15, 2026
0f85124
test(github): update fake PR fixtures to full V2 shape
vidhu-balad Jul 16, 2026
36f817e
test(github): add commit entry to fake PR fixtures
vidhu-balad Jul 16, 2026
ec85bb4
test(github): assert empty author is serialised in JSON, not omitted
vidhu-balad Jul 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion cmd/kosli/assertPRGithub_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,22 @@ func (suite *AssertPRGithubCommandTestSuite) SetupTest() {
ghUtils.NewGithubRetrieverFunc = func(token, baseURL, org, repository string, debug bool) types.PRRetriever {
return &ghUtils.FakeGitHubClient{
PRsByCommit: map[string][]*types.PREvidence{
suite.commitWithPR: {{URL: "https://github.com/kosli-dev/cli/pull/1", State: "MERGED"}},
suite.commitWithPR: {{
URL: "https://github.com/kosli-dev/cli/pull/1",
State: "MERGED",
Author: "test-user",
Title: "test PR",
CreatedAt: 1234567890,
HeadRef: "test-branch",
MergeCommit: suite.commitWithPR,
Commits: []types.Commit{{
SHA: suite.commitWithPR,
Message: "test commit",
Author: "Test User <test@example.com>",
Timestamp: 1234567890,
Branch: "test-branch",
}},
}},
},
}
}
Expand Down
17 changes: 16 additions & 1 deletion cmd/kosli/attestPRGithub_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,22 @@ func (suite *AttestGithubPRCommandTestSuite) SetupTest() {
ghUtils.NewGithubRetrieverFunc = func(token, baseURL, org, repository string, debug bool) types.PRRetriever {
return &ghUtils.FakeGitHubClient{
PRsByCommit: map[string][]*types.PREvidence{
suite.commitWithPR: {{URL: "https://github.com/kosli-dev/cli/pull/1", State: "MERGED"}},
suite.commitWithPR: {{
URL: "https://github.com/kosli-dev/cli/pull/1",
State: "MERGED",
Author: "test-user",
Title: "test PR",
CreatedAt: 1234567890,
HeadRef: "test-branch",
MergeCommit: suite.commitWithPR,
Commits: []types.Commit{{
SHA: suite.commitWithPR,
Message: "test commit",
Author: "Test User <test@example.com>",
Timestamp: 1234567890,
Branch: "test-branch",
}},
}},
},
}
}
Expand Down
28 changes: 28 additions & 0 deletions internal/github/build_pr_evidence_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package github

import (
"encoding/json"
"testing"
"time"

Expand Down Expand Up @@ -152,6 +153,33 @@ func TestBuildPREvidence_RecordsCommitSignature(t *testing.T) {
require.Equal(t, "VALID", *c.SignatureState)
}

// TestBuildPREvidence_EmptyAuthorIsPreserved verifies that when the PR
// creator's GitHub account has been deleted (Author.Login = ""), the empty
// string is preserved and serialised as "" rather than omitted, allowing the
// server to accept it directly.
func TestBuildPREvidence_EmptyAuthorIsPreserved(t *testing.T) {
evidence, err := buildPREvidence(
"https://github.com/kosli-dev/cli/pull/671",
"0e723254516c841126e81f76100be57258ff1386",
"MERGED",
"", // empty author — PR creator account deleted
"2026-03-01T09:00:00Z",
"2026-03-01T12:00:00Z",
"Fix something",
"fix-branch",
"main",
nil, nil,
)
require.NoError(t, err)
require.Equal(t, "", evidence.Author,
Comment on lines +173 to +174

@claude claude Bot Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 →

"empty PR author login must be preserved so it is serialised as an empty string, not omitted")
Comment on lines +173 to +175

@claude claude Bot Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


b, err := json.Marshal(evidence)
require.NoError(t, err)
require.Contains(t, string(b), `"author":""`,
"empty author must be serialised, not omitted")
}
Comment on lines +170 to +181

@claude claude Bot Jul 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 →


// TestBuildPREvidence_UnsignedCommitHasNoSignatureFields verifies an unsigned
// commit (no signature node) leaves verified/signature_state nil, so "unsigned"
// stays distinct from "present-but-invalid" (verified=false).
Expand Down
2 changes: 1 addition & 1 deletion internal/types/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ type PREvidence struct {
URL string `json:"url"`
State string `json:"state"`
Approvers []any `json:"approvers"`
Author string `json:"author,omitempty"`
Author string `json:"author"`

@claude claude Bot Jul 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

CreatedAt int64 `json:"created_at,omitempty"`
MergedAt int64 `json:"merged_at,omitempty"`
Title string `json:"title,omitempty"`
Expand Down
Loading