fix(github): send an empty label array rather than null

Once the mirror owns labels, a source pull request with none produced a nil
slice, which marshals to null, and GitHub answers 422: the field has to be
an array. Clearing labels is a legitimate and common request, so this broke
the update of nearly every mirrored pull request, and the label failure was
wrapped in a way that failed the whole pull request rather than just its
labels.

Found by the live suite on its first real run against GitHub, in the first
minute it existed. No fake had an opinion about null.

Author: anmolnagpal

CHANGELOG.md

@@ -21,6 +21,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - A failed state save on the fail-fast path was silent, so everything copied before the abort was copied again on the next run with no explanation
 - `git-sync` could not authenticate to Azure DevOps over git in `pat` mode, fixed in v0.2.1 and described there
 
+### Fixed
+- A mirrored pull request with no labels failed to update, taking the whole pull request with it. Once the mirror owned labels, an empty set was sent as null rather than as an empty array, and GitHub rejects that with 422. Most pull requests carry no labels, so this affected most of them. Found by the live suite on its first real run
+
 ### Added
 - A live provider test suite behind the `live` build tag, run with `make test-live`. It creates two throwaway repositories against a real GitHub account, mirrors a pull request between them, and checks what actually landed, then deletes them. Every other test in the repository checks SyncerD against fakes written alongside the code, which is how three defects that made pull request mirroring inoperable passed a green suite and a review
 

internal/livetest/live_test.go

@@ -154,6 +154,20 @@ func openPullRequest(t *testing.T, p *github.Provider, repoPath string) int {
 	return pr.Number
 }
 
+// relist reads the source pull requests again, as each run does, and
+// asserts how many are open.
+func relist(t *testing.T, p *github.Provider, repoPath string, want int) []vcs.PullRequest {
+	t.Helper()
+	prs, err := p.ListPullRequests(context.Background(), repoPath, vcs.PRListOptions{})
+	if err != nil {
+		t.Fatalf("list pull requests: %v", err)
+	}
+	if len(prs) != want {
+		t.Fatalf("got %d open pull requests, want %d", len(prs), want)
+	}
+	return prs
+}
+
 // TestGitHubToGitHubPullRequestRoundTrip mirrors a repository and its open
 // pull request to a second repository, against the real API.
 //
@@ -180,13 +194,7 @@ func TestGitHubToGitHubPullRequestRoundTrip(t *testing.T) {
 	t.Logf("source pull request %s#%d", srcPath, number)
 
 	// Read it back the way a run would.
-	prs, err := p.ListPullRequests(ctx, srcPath, vcs.PRListOptions{})
-	if err != nil {
-		t.Fatalf("list pull requests: %v", err)
-	}
-	if len(prs) != 1 {
-		t.Fatalf("got %d open pull requests, want 1", len(prs))
-	}
+	prs := relist(t, p, srcPath, 1)
 
 	// Mirror the branches first: a destination pull request cannot
 	// reference commits that have not arrived.
@@ -248,6 +256,7 @@ func TestGitHubToGitHubPullRequestRoundTrip(t *testing.T) {
 	}
 
 	// A second run must change nothing.
+	prs = relist(t, p, srcPath, 1)
 	res, err = prsync.Sync(ctx, prs, prsync.Options{
 		Mirror: "live", SourceRepo: srcPath, DestRepo: p.QualifiedPath(dstName),
 		BranchPrefix: "syncerd/pr", Source: p, Dest: p, SourceConv: p, DestConv: p,
@@ -260,14 +269,19 @@ func TestGitHubToGitHubPullRequestRoundTrip(t *testing.T) {
 		t.Errorf("a second run created %d pull requests; it must create none", res.Created)
 	}
 
-	// A comment at the source must reach the destination, and survive an
-	// edit without being duplicated.
+	// A comment at the source must reach the destination, exactly once.
 	commentID, err := p.CreateComment(ctx, srcPath, number, "a comment from the live test")
 	if err != nil {
 		t.Fatalf("comment at the source: %v", err)
 	}
 	t.Logf("source comment %s", commentID)
 
+	// Re-list, the way a run does. The engine reads the source afresh every
+	// time, and the watermark deliberately skips a pull request whose
+	// reported timestamp has not moved: reusing the list captured before
+	// the comment would test a situation that never occurs.
+	prs = relist(t, p, srcPath, 1)
+
 	res, err = prsync.Sync(ctx, prs, prsync.Options{
 		Mirror: "live", SourceRepo: srcPath, DestRepo: p.QualifiedPath(dstName),
 		BranchPrefix: "syncerd/pr", Source: p, Dest: p, SourceConv: p, DestConv: p,

internal/vcs/github/prwrite.go

@@ -134,6 +134,12 @@ func (p *Provider) setState(ctx context.Context, repoPath string, number int, st
 // that does not exist yet as a side effect of this call, so no separate
 // creation step is needed.
 func (p *Provider) setLabels(ctx context.Context, repoPath string, number int, labels []string) error {
+	// A nil slice marshals to null, and GitHub answers 422 for it: the
+	// field must be an array. Clearing the labels is a legitimate request,
+	// and it is the common one, since most pull requests carry none.
+	if labels == nil {
+		labels = []string{}
+	}
 	payload := map[string]any{"labels": labels}
 	_, _, err := p.do(ctx, http.MethodPut,
 		fmt.Sprintf("%s/repos/%s/issues/%d/labels", p.apiURL, repoPath, number), payload)

internal/vcs/github/prwrite_test.go

@@ -181,3 +181,30 @@ func TestReopenPullRequest(t *testing.T) {
 		t.Errorf("state = %v, want open", payload["state"])
 	}
 }
+
+func TestClearingLabelsSendsAnEmptyArrayNotNull(t *testing.T) {
+	// A source pull request with no labels is the common case. A nil slice
+	// marshals to null, which GitHub rejects with 422, and that failed the
+	// whole pull request rather than just its labels.
+	var raw map[string]json.RawMessage
+
+	mux := http.NewServeMux()
+	mux.HandleFunc("/repos/acme/widget/pulls/12", func(w http.ResponseWriter, r *http.Request) {
+		writeJSON(w, map[string]any{"number": 12})
+	})
+	mux.HandleFunc("/repos/acme/widget/issues/12/labels", func(w http.ResponseWriter, r *http.Request) {
+		_ = json.NewDecoder(r.Body).Decode(&raw)
+		writeJSON(w, []map[string]any{})
+	})
+
+	p, _ := newProvider(t, mux)
+	err := p.UpdatePullRequest(context.Background(), "acme/widget", 12, vcs.PullRequestSpec{
+		Title: "Add login", BaseBranch: "main", SyncLabels: true, Labels: nil,
+	})
+	if err != nil {
+		t.Fatalf("update: %v", err)
+	}
+	if got := string(raw["labels"]); got != "[]" {
+		t.Errorf("labels = %s, want an empty array", got)
+	}
+}