From c912f3b1e18c1bd49b31553106d9905b99837fbe Mon Sep 17 00:00:00 2001 From: mintaka Date: Sun, 11 Oct 2026 16:56:46 -0400 Subject: [PATCH] feat(forge): carry PR head SHA, mergeable state and review commit id (RIG-5182) GitHub's pull detail and review rows already return head.sha, mergeable_state and commit_id. Decode them into forge.PullRequest and forge.Review, and copy them into the head_sha, mergeable_state and commit_id proto fields so the stored pr JSONB carries them. Co-authored-by: Matt Wilkinson --- go/internal/board/issue_prs_pgtest_test.go | 39 ++++++++++++ go/internal/forge/github.go | 63 ++++++++++--------- go/internal/forge/github_test.go | 12 ++-- go/internal/forge/provider.go | 6 ++ .../testdata/github/create_pull_request.json | 2 + .../testdata/github/get_pull_request.json | 11 +++- .../github/transition_pull_request_close.json | 3 + .../transition_pull_request_reopen.json | 3 + go/internal/forge/translate.go | 29 +++++---- go/internal/forge/translate_test.go | 30 +++++---- 10 files changed, 137 insertions(+), 61 deletions(-) diff --git a/go/internal/board/issue_prs_pgtest_test.go b/go/internal/board/issue_prs_pgtest_test.go index 9205add70..ae0f4be13 100644 --- a/go/internal/board/issue_prs_pgtest_test.go +++ b/go/internal/board/issue_prs_pgtest_test.go @@ -143,6 +143,45 @@ func TestPullRequestBeforeIssueAttachesLater(t *testing.T) { } } +func TestPullRequestFieldsSurviveStoredPRRoundTrip(t *testing.T) { + p, _, _ := newIssueBoard(t) + mustPublishIssue(t, p, 1) + + current := ingestedPR(10, 0, time.Hour, 1) + current.PR.HeadSha = "head-sha" + current.PR.MergeableState = "blocked" + current.PR.Reviews = []*compassv1.Review{{CommitId: "review-sha"}} + mustPublishPR(t, p, current) + + legacy := ingestedPR(11, time.Hour, 2*time.Hour, 1) + mustPublishPR(t, p, legacy) + + prs := cachedIssue1(t, p).GetPrs() + if len(prs) != 2 { + t.Fatalf("stored pull requests = %d, want 2", len(prs)) + } + byNumber := make(map[uint32]*compassv1.PullRequest, len(prs)) + for _, pr := range prs { + byNumber[pr.GetNumber()] = pr + } + currentPR := byNumber[10] + if currentPR == nil { + t.Fatal("stored current pull request is missing") + } + if currentPR.GetHeadSha() != "head-sha" || currentPR.GetMergeableState() != "blocked" { + t.Errorf("stored PR head/mergeable = %q/%q, want head-sha/blocked", currentPR.GetHeadSha(), currentPR.GetMergeableState()) + } + if len(currentPR.GetReviews()) != 1 || currentPR.GetReviews()[0].GetCommitId() != "review-sha" { + t.Errorf("stored PR reviews = %v, want one review on review-sha", currentPR.GetReviews()) + } + legacyPR := byNumber[11] + if legacyPR == nil { + t.Fatal("stored legacy pull request is missing") + } + if legacyPR.GetHeadSha() != "" || legacyPR.GetMergeableState() != "" || len(legacyPR.GetReviews()) != 0 { + t.Errorf("legacy stored PR fields = %q/%q/%v, want empty values", legacyPR.GetHeadSha(), legacyPR.GetMergeableState(), legacyPR.GetReviews()) + } +} func TestFallbackMovesWhenExplicitTargetArrives(t *testing.T) { ctx := context.Background() p, bus, st := newIssueBoard(t) diff --git a/go/internal/forge/github.go b/go/internal/forge/github.go index 79cb9a8ea..0b3a58515 100644 --- a/go/internal/forge/github.go +++ b/go/internal/forge/github.go @@ -567,19 +567,20 @@ func (g *GitHub) GetIssue(ctx context.Context, repo string, number uint64) (Issu // open|closed plus a separate merged bool; toPullRequest folds them into the // forge domain's open|closed|merged. type ghPullDetail struct { - Number uint64 `json:"number"` - Title string `json:"title"` - Body string `json:"body"` - State string `json:"state"` - HTMLURL string `json:"html_url"` - Draft bool `json:"draft"` - Additions uint32 `json:"additions"` - Deletions uint32 `json:"deletions"` - ChangedFiles uint32 `json:"changed_files"` - Merged bool `json:"merged"` - CreatedAt string `json:"created_at"` - UpdatedAt string `json:"updated_at"` - Head struct { + Number uint64 `json:"number"` + Title string `json:"title"` + Body string `json:"body"` + State string `json:"state"` + HTMLURL string `json:"html_url"` + Draft bool `json:"draft"` + Additions uint32 `json:"additions"` + Deletions uint32 `json:"deletions"` + ChangedFiles uint32 `json:"changed_files"` + Merged bool `json:"merged"` + MergeableState string `json:"mergeable_state"` + CreatedAt string `json:"created_at"` + UpdatedAt string `json:"updated_at"` + Head struct { Ref string `json:"ref"` SHA string `json:"sha"` } `json:"head"` @@ -601,15 +602,17 @@ func (r ghPullDetail) toPullRequest() PullRequest { state = prStateMerged } return PullRequest{ - Number: r.Number, - Title: r.Title, - Body: r.Body, - State: state, - URL: r.HTMLURL, - HeadRef: r.Head.Ref, - BaseRef: r.Base.Ref, - ForgeAccount: r.User.Login, - Draft: r.Draft, + Number: r.Number, + Title: r.Title, + Body: r.Body, + State: state, + URL: r.HTMLURL, + HeadRef: r.Head.Ref, + HeadSHA: r.Head.SHA, + MergeableState: r.MergeableState, + BaseRef: r.Base.Ref, + ForgeAccount: r.User.Login, + Draft: r.Draft, Changed: ChangedStats{ Files: r.ChangedFiles, Additions: r.Additions, @@ -623,9 +626,10 @@ func (r ghPullDetail) toPullRequest() PullRequest { // ghReviewRow is the wire shape of one entry in the pull reviews list. Only the // fields forge.Review needs are decoded; the user type discriminates a bot. type ghReviewRow struct { - Body string `json:"body"` - State string `json:"state"` - User struct { + Body string `json:"body"` + State string `json:"state"` + CommitID string `json:"commit_id"` + User struct { Login string `json:"login"` Type string `json:"type"` } `json:"user"` @@ -656,10 +660,11 @@ func (g *GitHub) GetPullRequest(ctx context.Context, repo string, number uint64) pr.Reviews = make([]Review, 0, len(reviews)) for _, rv := range reviews { pr.Reviews = append(pr.Reviews, Review{ - Author: rv.User.Login, - IsBot: rv.User.Type == "Bot", - Verdict: strings.ToLower(rv.State), - Body: rv.Body, + Author: rv.User.Login, + IsBot: rv.User.Type == "Bot", + Verdict: strings.ToLower(rv.State), + Body: rv.Body, + CommitID: rv.CommitID, }) } diff --git a/go/internal/forge/github_test.go b/go/internal/forge/github_test.go index 03a6e24c0..bfda8f4cd 100644 --- a/go/internal/forge/github_test.go +++ b/go/internal/forge/github_test.go @@ -1461,13 +1461,14 @@ func TestGetPullRequestHappy(t *testing.T) { "deletions": 30, "changed_files": 4, "merged": true, + "mergeable_state": "blocked", "head": {"ref": "feature", "sha": "abc123"}, "base": {"ref": "main"}, "user": {"login": "octocat"} }` const reviewsBody = `[ - {"body": "lgtm", "state": "APPROVED", "user": {"login": "botly", "type": "Bot"}}, - {"body": "needs work", "state": "CHANGES_REQUESTED", "user": {"login": "carol", "type": "User"}} + {"body": "lgtm", "state": "APPROVED", "commit_id": "review-sha", "user": {"login": "botly", "type": "Bot"}}, + {"body": "needs work", "state": "CHANGES_REQUESTED", "commit_id": "old-sha", "user": {"login": "carol", "type": "User"}} ]` const checkRunsBody = `{"check_runs": [ {"name": "build", "status": "completed", "conclusion": "success", "html_url": "https://ci/build"} @@ -1537,12 +1538,15 @@ func TestGetPullRequestHappy(t *testing.T) { if len(got.Reviews) != 2 { t.Fatalf("Reviews len = %d, want 2", len(got.Reviews)) } - if got.Reviews[0] != (Review{Author: "botly", IsBot: true, Verdict: "approved", Body: "lgtm"}) { + if got.Reviews[0] != (Review{Author: "botly", IsBot: true, Verdict: "approved", Body: "lgtm", CommitID: "review-sha"}) { t.Errorf("Reviews[0] = %+v", got.Reviews[0]) } - if got.Reviews[1] != (Review{Author: "carol", IsBot: false, Verdict: "changes_requested", Body: "needs work"}) { + if got.Reviews[1] != (Review{Author: "carol", IsBot: false, Verdict: "changes_requested", Body: "needs work", CommitID: "old-sha"}) { t.Errorf("Reviews[1] = %+v", got.Reviews[1]) } + if got.HeadSHA != "abc123" || got.MergeableState != "blocked" { + t.Errorf("HeadSHA/MergeableState = %q/%q, want abc123/blocked", got.HeadSHA, got.MergeableState) + } // Embedded checks populated (both sources), rolled up to success; only the // check GraphQL reports as required is marked Required. diff --git a/go/internal/forge/provider.go b/go/internal/forge/provider.go index 70edf1813..54a9b75b3 100644 --- a/go/internal/forge/provider.go +++ b/go/internal/forge/provider.go @@ -98,6 +98,10 @@ type PullRequest struct { HeadRef string // BaseRef is the target branch name. BaseRef string + // HeadSHA is the commit currently at the source branch head. + HeadSHA string + // MergeableState is the forge's mergeability assessment for the PR. + MergeableState string // ForgeAccount is the native forge login that authored the PR. ForgeAccount string // Draft reports whether the PR is a draft on the forge. @@ -160,6 +164,8 @@ type Review struct { Verdict string // Body is the review's summary body. Body string + // CommitID is the commit the reviewer submitted this review against. + CommitID string } // ReviewThread is one inline (or PR-level) review comment thread. diff --git a/go/internal/forge/testdata/github/create_pull_request.json b/go/internal/forge/testdata/github/create_pull_request.json index f345f3162..7615ad237 100644 --- a/go/internal/forge/testdata/github/create_pull_request.json +++ b/go/internal/forge/testdata/github/create_pull_request.json @@ -41,6 +41,8 @@ "URL": "https://github.com/org/repo/pull/13", "HeadRef": "feature", "BaseRef": "main", + "HeadSHA": "", + "MergeableState": "", "ForgeAccount": "octocat", "Draft": true, "Changed": { "Files": 0, "Additions": 0, "Deletions": 0 }, diff --git a/go/internal/forge/testdata/github/get_pull_request.json b/go/internal/forge/testdata/github/get_pull_request.json index d40b3dbe9..316d9269c 100644 --- a/go/internal/forge/testdata/github/get_pull_request.json +++ b/go/internal/forge/testdata/github/get_pull_request.json @@ -20,6 +20,7 @@ "deletions": 30, "changed_files": 4, "merged": true, + "mergeable_state": "blocked", "head": { "ref": "feature", "sha": "abc123" }, "base": { "ref": "main" }, "user": { "login": "octocat" } @@ -31,11 +32,13 @@ { "body": "lgtm", "state": "APPROVED", + "commit_id": "review-sha", "user": { "login": "botly", "type": "Bot" } }, { "body": "needs work", "state": "CHANGES_REQUESTED", + "commit_id": "old-sha", "user": { "login": "carol", "type": "User" } } ] @@ -165,6 +168,8 @@ "State": "merged", "URL": "https://github.com/org/repo/pull/42", "HeadRef": "feature", + "HeadSHA": "abc123", + "MergeableState": "blocked", "BaseRef": "main", "ForgeAccount": "octocat", "Draft": false, @@ -192,13 +197,15 @@ "Author": "botly", "IsBot": true, "Verdict": "approved", - "Body": "lgtm" + "Body": "lgtm", + "CommitID": "review-sha" }, { "Author": "carol", "IsBot": false, "Verdict": "changes_requested", - "Body": "needs work" + "Body": "needs work", + "CommitID": "old-sha" } ], "Threads": [ diff --git a/go/internal/forge/testdata/github/transition_pull_request_close.json b/go/internal/forge/testdata/github/transition_pull_request_close.json index 4d28a27f0..a69b6d779 100644 --- a/go/internal/forge/testdata/github/transition_pull_request_close.json +++ b/go/internal/forge/testdata/github/transition_pull_request_close.json @@ -30,6 +30,7 @@ "deletions": 2, "changed_files": 3, "merged": false, + "mergeable_state": "blocked", "head": { "ref": "feature", "sha": "abc123" }, "base": { "ref": "main" }, "user": { "login": "octocat" } @@ -41,6 +42,8 @@ "State": "closed", "URL": "https://github.com/org/repo/pull/7", "HeadRef": "feature", + "HeadSHA": "abc123", + "MergeableState": "blocked", "BaseRef": "main", "ForgeAccount": "octocat", "Draft": false, diff --git a/go/internal/forge/testdata/github/transition_pull_request_reopen.json b/go/internal/forge/testdata/github/transition_pull_request_reopen.json index 20afae2cb..1fda53c19 100644 --- a/go/internal/forge/testdata/github/transition_pull_request_reopen.json +++ b/go/internal/forge/testdata/github/transition_pull_request_reopen.json @@ -30,6 +30,7 @@ "deletions": 2, "changed_files": 3, "merged": false, + "mergeable_state": "blocked", "head": { "ref": "feature", "sha": "abc123" }, "base": { "ref": "main" }, "user": { "login": "octocat" } @@ -41,6 +42,8 @@ "State": "open", "URL": "https://github.com/org/repo/pull/7", "HeadRef": "feature", + "HeadSHA": "abc123", + "MergeableState": "blocked", "BaseRef": "main", "ForgeAccount": "octocat", "Draft": false, diff --git a/go/internal/forge/translate.go b/go/internal/forge/translate.go index 4e42c21ea..887b627a8 100644 --- a/go/internal/forge/translate.go +++ b/go/internal/forge/translate.go @@ -69,15 +69,17 @@ func TranslateIssue(in Issue, attr *compassv1.AgentAttribution) *compassv1.Issue // caller/projection supplies them). func TranslatePullRequest(in PullRequest, attr *compassv1.AgentAttribution) *compassv1.PullRequest { return &compassv1.PullRequest{ - Number: narrowNumber(in.Number), - Title: in.Title, - ForgeState: in.State, - Url: in.URL, - HeadRef: in.HeadRef, - BaseRef: in.BaseRef, - ForgeAccount: in.ForgeAccount, - Draft: in.Draft, - Agent: attr, + Number: narrowNumber(in.Number), + Title: in.Title, + ForgeState: in.State, + Url: in.URL, + HeadRef: in.HeadRef, + BaseRef: in.BaseRef, + HeadSha: in.HeadSHA, + MergeableState: in.MergeableState, + ForgeAccount: in.ForgeAccount, + Draft: in.Draft, + Agent: attr, Changed: &compassv1.ChangedStats{ Files: in.Changed.Files, Additions: in.Changed.Additions, @@ -131,10 +133,11 @@ func translateReviews(in []Review) []*compassv1.Review { out := make([]*compassv1.Review, len(in)) for i, r := range in { out[i] = &compassv1.Review{ - Author: r.Author, - IsBot: r.IsBot, - Verdict: r.Verdict, - Body: r.Body, + Author: r.Author, + IsBot: r.IsBot, + Verdict: r.Verdict, + Body: r.Body, + CommitId: r.CommitID, } } return out diff --git a/go/internal/forge/translate_test.go b/go/internal/forge/translate_test.go index acf89fa25..279bc73a7 100644 --- a/go/internal/forge/translate_test.go +++ b/go/internal/forge/translate_test.go @@ -172,8 +172,8 @@ func assertPRReviews(t *testing.T, got *compassv1.PullRequest) { t.Fatalf("Reviews len = %d, want 1", len(got.GetReviews())) } r := got.GetReviews()[0] - if r.GetAuthor() != "rev" || !r.GetIsBot() || r.GetVerdict() != "approved" || r.GetBody() != "lgtm" { - t.Errorf("Review = %+v, want {rev bot approved lgtm}", r) + if r.GetAuthor() != "rev" || !r.GetIsBot() || r.GetVerdict() != "approved" || r.GetBody() != "lgtm" || r.GetCommitId() != "review-sha" { + t.Errorf("Review = %+v, want {rev bot approved lgtm review-sha}", r) } } @@ -198,22 +198,23 @@ func assertPRThreads(t *testing.T, got *compassv1.PullRequest) { func TestTranslatePullRequestRoundTrip(t *testing.T) { attr := &compassv1.AgentAttribution{AgentHandle: "agent-pr"} in := PullRequest{ - Number: 7, - Title: "a pr", - Body: "\npr body", - State: "merged", - URL: "https://forge/pull/7", - HeadRef: "feature", - BaseRef: "main", - ForgeAccount: "octocat", - Draft: true, - Changed: ChangedStats{Files: 3, Additions: 10, Deletions: 2}, + Number: 7, + Title: "a pr", + State: "merged", + URL: "https://forge/pull/7", + HeadRef: "feature", + BaseRef: "main", + HeadSHA: "head-sha", + MergeableState: "blocked", + ForgeAccount: "octocat", + Draft: true, + Changed: ChangedStats{Files: 3, Additions: 10, Deletions: 2}, Checks: Checks{ HeadSHA: "abc123", State: "success", Checks: []Check{{Name: "ci", State: "success", URL: "https://ci/1", Required: true}}, }, - Reviews: []Review{{Author: "rev", IsBot: true, Verdict: "approved", Body: "lgtm"}}, + Reviews: []Review{{Author: "rev", IsBot: true, Verdict: "approved", Body: "lgtm", CommitID: "review-sha"}}, Threads: []ReviewThread{{ Path: "file.go", Resolved: true, @@ -238,6 +239,9 @@ func TestTranslatePullRequestRoundTrip(t *testing.T) { if got.GetHeadRef() != "feature" { t.Errorf("HeadRef = %q, want %q", got.GetHeadRef(), "feature") } + if got.GetHeadSha() != "head-sha" || got.GetMergeableState() != "blocked" { + t.Errorf("HeadSha/MergeableState = %q/%q, want head-sha/blocked", got.GetHeadSha(), got.GetMergeableState()) + } if got.GetBaseRef() != "main" { t.Errorf("BaseRef = %q, want %q", got.GetBaseRef(), "main") }