From d2235e9ae0b092a5b593b65494b1a9b6118923ce Mon Sep 17 00:00:00 2001 From: Yulian Diaz <5605867+spatialy@users.noreply.github.com> Date: Mon, 5 Oct 2026 14:11:32 -0400 Subject: [PATCH 1/2] fix(backend): treat GitHub PRs with merged_at as merged GitHub's list endpoint (GET /repos/{owner}/{repo}/pulls), which the SCM sync uses, omits the `merged` field and only returns `merged_at`. Every listed PR therefore converted with IsMerged = false, so sync stored merged PRs as "closed", never emitted scm_pr_merged, and skipped smart commits. Count a non-nil MergedAt as merged as well. The single-PR endpoint keeps working as before because it sets both fields. Fixes #304 --- .gitignore | 1 + internal/scm/github.go | 9 +++- internal/scm/github_pull_request_test.go | 63 ++++++++++++++++++++++++ 3 files changed, 72 insertions(+), 1 deletion(-) create mode 100644 internal/scm/github_pull_request_test.go diff --git a/.gitignore b/.gitignore index ad6e408c2..ab62a7dde 100644 --- a/.gitignore +++ b/.gitignore @@ -22,6 +22,7 @@ cx # overlaid onto this checkout by its runners. *_test.go !/internal/database/gitlab_schema_test.go +!/internal/scm/github_pull_request_test.go !/internal/scm/gitlab_test.go !/internal/services/planning_milestones_release_test.go !/internal/utils/repository_path_test.go diff --git a/internal/scm/github.go b/internal/scm/github.go index 8905c1a3a..b5dbdd2cd 100644 --- a/internal/scm/github.go +++ b/internal/scm/github.go @@ -1395,6 +1395,13 @@ type githubPullRequest struct { ClosedAt *time.Time `json:"closed_at"` } +// toPullRequest converts a GitHub pull request to the provider-neutral type. +// +// GitHub's list endpoint (GET /repos/{owner}/{repo}/pulls) omits the `merged` +// field, so Merged is always false for listed PRs; only the single-PR endpoint +// sets it. `merged_at` is present on both, so a non-nil MergedAt also counts as +// merged. Without it, sync stored merged PRs as "closed" and never emitted +// scm_pr_merged or ran smart commits (Windshiftapp/core#304). func (pr githubPullRequest) toPullRequest() PullRequest { return PullRequest{ ID: pr.ID, @@ -1407,7 +1414,7 @@ func (pr githubPullRequest) toPullRequest() PullRequest { HeadRepo: pr.Head.Repo.FullName, HeadSHA: pr.Head.SHA, BaseBranch: pr.Base.Ref, - IsMerged: pr.Merged, + IsMerged: pr.Merged || pr.MergedAt != nil, IsDraft: pr.Draft, Author: pr.User.toUser(), CreatedAt: pr.CreatedAt, diff --git a/internal/scm/github_pull_request_test.go b/internal/scm/github_pull_request_test.go new file mode 100644 index 000000000..3c3631288 --- /dev/null +++ b/internal/scm/github_pull_request_test.go @@ -0,0 +1,63 @@ +package scm + +import ( + "context" + "net/http" + "testing" + "time" + + "windshift/internal/models" +) + +// GitHub's list endpoint omits `merged` and only carries `merged_at` +// (Windshiftapp/core#304). A listed merged PR must still read as merged, or +// sync stores it as "closed" and never emits scm_pr_merged. +func TestGitHubListPullRequestsDetectsMergedFromMergedAt(t *testing.T) { + t.Parallel() + provider, err := NewGitHubProvider(ProviderConfig{AuthMethod: models.SCMAuthMethodPAT, PersonalAccessToken: "pat-token"}) + if err != nil { + t.Fatal(err) + } + provider.httpClient = &http.Client{Transport: roundTripFunc(func(request *http.Request) (*http.Response, error) { + if request.URL.Path != "/repos/acme/app/pulls" { + t.Fatalf("unexpected path: %s", request.URL.Path) + } + // Shapes as returned by GET /repos/{owner}/{repo}/pulls: no "merged" key. + return jsonResponse(http.StatusOK, `[ + {"id": 1, "number": 11, "state": "closed", "merged_at": "2026-10-01T12:00:00Z", "closed_at": "2026-10-01T12:00:00Z"}, + {"id": 2, "number": 12, "state": "closed", "merged_at": null, "closed_at": "2026-10-01T12:00:00Z"}, + {"id": 3, "number": 13, "state": "open", "merged_at": null} + ]`), nil + })} + + prs, err := provider.ListPullRequests(context.Background(), "acme", "app", ListPROptions{State: "all"}) + if err != nil { + t.Fatal(err) + } + if len(prs) != 3 { + t.Fatalf("got %d pull requests, want 3", len(prs)) + } + + merged, closed, open := prs[0], prs[1], prs[2] + if !merged.IsMerged { + t.Errorf("PR #%d with merged_at set: IsMerged = false, want true", merged.Number) + } + if want := time.Date(2026, 10, 1, 12, 0, 0, 0, time.UTC); merged.MergedAt == nil || !merged.MergedAt.Equal(want) { + t.Errorf("PR #%d MergedAt = %v, want %v", merged.Number, merged.MergedAt, want) + } + if closed.IsMerged { + t.Errorf("PR #%d closed without merge: IsMerged = true, want false", closed.Number) + } + if open.IsMerged { + t.Errorf("PR #%d open: IsMerged = true, want false", open.Number) + } +} + +// The single-PR endpoint does send `merged`; it keeps working on its own. +func TestGitHubPullRequestMergedFlagStillHonoured(t *testing.T) { + t.Parallel() + pr := githubPullRequest{Number: 21, State: "closed", Merged: true} + if !pr.toPullRequest().IsMerged { + t.Fatal("merged=true without merged_at: IsMerged = false, want true") + } +} From cd36d6ec5976de885d56fbd88adbaa3b8ca5766e Mon Sep 17 00:00:00 2001 From: Stefan Ernst <24554551+stefan-ernst@users.noreply.github.com> Date: Mon, 5 Oct 2026 22:17:44 +0200 Subject: [PATCH 2/2] test(scm): move GitHub merged PR regression test to core-tests --- .gitignore | 1 - internal/scm/github_pull_request_test.go | 63 ------------------------ 2 files changed, 64 deletions(-) delete mode 100644 internal/scm/github_pull_request_test.go diff --git a/.gitignore b/.gitignore index ab62a7dde..ad6e408c2 100644 --- a/.gitignore +++ b/.gitignore @@ -22,7 +22,6 @@ cx # overlaid onto this checkout by its runners. *_test.go !/internal/database/gitlab_schema_test.go -!/internal/scm/github_pull_request_test.go !/internal/scm/gitlab_test.go !/internal/services/planning_milestones_release_test.go !/internal/utils/repository_path_test.go diff --git a/internal/scm/github_pull_request_test.go b/internal/scm/github_pull_request_test.go deleted file mode 100644 index 3c3631288..000000000 --- a/internal/scm/github_pull_request_test.go +++ /dev/null @@ -1,63 +0,0 @@ -package scm - -import ( - "context" - "net/http" - "testing" - "time" - - "windshift/internal/models" -) - -// GitHub's list endpoint omits `merged` and only carries `merged_at` -// (Windshiftapp/core#304). A listed merged PR must still read as merged, or -// sync stores it as "closed" and never emits scm_pr_merged. -func TestGitHubListPullRequestsDetectsMergedFromMergedAt(t *testing.T) { - t.Parallel() - provider, err := NewGitHubProvider(ProviderConfig{AuthMethod: models.SCMAuthMethodPAT, PersonalAccessToken: "pat-token"}) - if err != nil { - t.Fatal(err) - } - provider.httpClient = &http.Client{Transport: roundTripFunc(func(request *http.Request) (*http.Response, error) { - if request.URL.Path != "/repos/acme/app/pulls" { - t.Fatalf("unexpected path: %s", request.URL.Path) - } - // Shapes as returned by GET /repos/{owner}/{repo}/pulls: no "merged" key. - return jsonResponse(http.StatusOK, `[ - {"id": 1, "number": 11, "state": "closed", "merged_at": "2026-10-01T12:00:00Z", "closed_at": "2026-10-01T12:00:00Z"}, - {"id": 2, "number": 12, "state": "closed", "merged_at": null, "closed_at": "2026-10-01T12:00:00Z"}, - {"id": 3, "number": 13, "state": "open", "merged_at": null} - ]`), nil - })} - - prs, err := provider.ListPullRequests(context.Background(), "acme", "app", ListPROptions{State: "all"}) - if err != nil { - t.Fatal(err) - } - if len(prs) != 3 { - t.Fatalf("got %d pull requests, want 3", len(prs)) - } - - merged, closed, open := prs[0], prs[1], prs[2] - if !merged.IsMerged { - t.Errorf("PR #%d with merged_at set: IsMerged = false, want true", merged.Number) - } - if want := time.Date(2026, 10, 1, 12, 0, 0, 0, time.UTC); merged.MergedAt == nil || !merged.MergedAt.Equal(want) { - t.Errorf("PR #%d MergedAt = %v, want %v", merged.Number, merged.MergedAt, want) - } - if closed.IsMerged { - t.Errorf("PR #%d closed without merge: IsMerged = true, want false", closed.Number) - } - if open.IsMerged { - t.Errorf("PR #%d open: IsMerged = true, want false", open.Number) - } -} - -// The single-PR endpoint does send `merged`; it keeps working on its own. -func TestGitHubPullRequestMergedFlagStillHonoured(t *testing.T) { - t.Parallel() - pr := githubPullRequest{Number: 21, State: "closed", Merged: true} - if !pr.toPullRequest().IsMerged { - t.Fatal("merged=true without merged_at: IsMerged = false, want true") - } -}