From a6fa382d00b1e670885ff2e384d5066582b56c39 Mon Sep 17 00:00:00 2001 From: seal Date: Tue, 18 Aug 2026 21:58:25 -0400 Subject: [PATCH 1/2] feat(forge): GitHub SubmitReview + Provider.SubmitReview method (RIG-2170) Add the submit-review write path to the GitHub Provider: a new SubmitReview/ReviewCommentInput/SubmittedReview type triple and a SubmitReview method on the Provider interface, implemented for the GitHub client as POST /repos/{repo}/pulls/{number}/reviews through the shared doJSON write helper. The write-side verdict vocabulary (approve/request_changes/comment) maps to GitHub's reviews-POST event tokens (APPROVE/REQUEST_CHANGES/COMMENT); an unknown verdict is rejected before any wire call, and COMMENT/REQUEST_CHANGES require a non-empty body client-side while APPROVE may be bodyless. Empty inline-comments omit the array rather than sending null. Widens the Provider interface, so FakeProvider gains the method (the compile-break named in the write-path design A7); no out-of-package Provider implementor exists. Hermetic httptest coverage: request golden, event mapping, empty-comments, unknown-verdict/empty-body no-wire, bodyless approve, off-diff 422, and 403/404 error mapping. Refs RIG-2208, RIG-2170. Co-authored-by: Matt Wilkinson --- go/internal/forge/fake.go | 10 ++ go/internal/forge/github.go | 69 +++++++++++ go/internal/forge/github_test.go | 193 +++++++++++++++++++++++++++++++ go/internal/forge/provider.go | 29 ++++- 4 files changed, 300 insertions(+), 1 deletion(-) diff --git a/go/internal/forge/fake.go b/go/internal/forge/fake.go index 74f37ce5..d4c5aa90 100644 --- a/go/internal/forge/fake.go +++ b/go/internal/forge/fake.go @@ -53,6 +53,8 @@ type FakeProvider struct { GetPRResult PullRequest // ChecksResult is returned by Checks when no error is scripted. ChecksResult Checks + // SubmitReviewResult is returned by SubmitReview when no error is scripted. + SubmitReviewResult SubmittedReview // BodyLimitResult is returned by BodyLimit; 0 (the default) means unlimited. BodyLimitResult int @@ -149,6 +151,14 @@ func (f *FakeProvider) CommentOnPullRequest(_ context.Context, repo string, numb return f.CommentResult, nil } +// SubmitReview records the call and returns the scripted result or error. +func (f *FakeProvider) SubmitReview(_ context.Context, repo string, number uint64, in SubmitReview) (SubmittedReview, error) { + if err := f.record(Call{Method: "SubmitReview", Repo: repo, Number: number, Payload: in}); err != nil { + return SubmittedReview{}, err + } + return f.SubmitReviewResult, nil +} + // GetPullRequest records the call and returns the scripted result or error. func (f *FakeProvider) GetPullRequest(_ context.Context, repo string, number uint64) (PullRequest, error) { if err := f.record(Call{Method: "GetPullRequest", Repo: repo, Number: number}); err != nil { diff --git a/go/internal/forge/github.go b/go/internal/forge/github.go index 82b842a7..fc0ad017 100644 --- a/go/internal/forge/github.go +++ b/go/internal/forge/github.go @@ -362,6 +362,75 @@ func (g *GitHub) CommentOnPullRequest(ctx context.Context, repo string, number u return out.toComment(), nil } +// reviewEvent maps a write-side verdict to its GitHub reviews-POST event token +// and whether GitHub requires a non-empty body for it. An unknown verdict is +// absent from reviewEvents and rejected before any wire call (design §T3); +// APPROVE may be bodyless (A2), COMMENT and REQUEST_CHANGES may not. +type reviewEvent struct { + token string + requiresBody bool +} + +// The write-side verdict vocabulary (design §T3): GitHub's reviews-POST event +// token set, distinct from the past-tense read-side Review.verdict. +const ( + VerdictApprove = "approve" + VerdictRequestChanges = "request_changes" + VerdictComment = "comment" +) + +var reviewEvents = map[string]reviewEvent{ + VerdictApprove: {token: "APPROVE", requiresBody: false}, + VerdictRequestChanges: {token: "REQUEST_CHANGES", requiresBody: true}, + VerdictComment: {token: "COMMENT", requiresBody: true}, +} + +// ghReviewComment is the wire shape of one inline comment inside a reviews POST. +type ghReviewComment struct { + Path string `json:"path"` + Line uint32 `json:"line"` + Side string `json:"side,omitempty"` + Body string `json:"body"` +} + +// ghReview is the wire shape of the reviews-POST 201 response. Only the fields +// forge.SubmittedReview needs are decoded. +type ghReview struct { + ID uint64 `json:"id"` + HTMLURL string `json:"html_url"` +} + +// SubmitReview submits a pull-request review on PR number in repo. in.Body is +// PRE-stamped by the Service; in.Comments ride unstamped inside the review. The +// verdict is validated (and, for COMMENT/REQUEST_CHANGES, a non-empty body +// required — GitHub rejects a bodyless one; APPROVE may be bodyless per A2) +// BEFORE any wire call. Maps to POST /repos/{repo}/pulls/{number}/reviews. +func (g *GitHub) SubmitReview(ctx context.Context, repo string, number uint64, in SubmitReview) (SubmittedReview, error) { + ev, ok := reviewEvents[in.Verdict] + if !ok { + return SubmittedReview{}, fmt.Errorf("forge: github submit review %q#%d: unknown verdict %q", repo, number, in.Verdict) + } + if in.Body == "" && ev.requiresBody { + return SubmittedReview{}, fmt.Errorf("forge: github submit review %q#%d: verdict %q requires a body", repo, number, in.Verdict) + } + + body := struct { + Event string `json:"event"` + Body string `json:"body"` + Comments []ghReviewComment `json:"comments,omitempty"` + }{Event: ev.token, Body: in.Body} + for _, c := range in.Comments { + body.Comments = append(body.Comments, ghReviewComment(c)) + } + + url := g.apiBase() + "/repos/" + repo + "/pulls/" + strconv.FormatUint(number, 10) + "/reviews" + var out ghReview + if err := g.doJSON(ctx, url, body, &out); err != nil { + return SubmittedReview{}, fmt.Errorf("forge: github submit review %q#%d: %w", repo, number, err) + } + return SubmittedReview{ID: out.ID, URL: out.HTMLURL, Verdict: in.Verdict}, nil +} + // BodyLimit is the max issue/comment/PR body size the Service enforces before a // write, in BYTES. GitHub caps these bodies at 65536 CHARACTERS; because a // UTF-8 string's character count never exceeds its byte count, enforcing 65536 diff --git a/go/internal/forge/github_test.go b/go/internal/forge/github_test.go index 49223945..62ba60de 100644 --- a/go/internal/forge/github_test.go +++ b/go/internal/forge/github_test.go @@ -1006,3 +1006,196 @@ func TestGitHubGateConcurrentAccess(t *testing.T) { } wg.Wait() } + +// --- write path: SubmitReview through doJSON --------------------------------- + +// The request golden: verdict->event mapping, body, and the inline comments +// array shape (path/line/side/body). +func TestSubmitReviewRequestAndDecode(t *testing.T) { + const respBody = `{"id": 8801, "html_url": "https://github.com/org/repo/pull/13#pullrequestreview-8801"}` + rt := &scriptedRoundTripper{responses: []scriptedResponse{{status: 201, body: respBody}}} + g := newTestGitHub(rt, &fakeTokenSource{token: "sekret"}) + + got, err := g.SubmitReview(context.Background(), "org/repo", 13, SubmitReview{ + Verdict: "request_changes", + Body: "please fix", + Comments: []ReviewCommentInput{ + {Path: "a.go", Line: 12, Side: "RIGHT", Body: "here"}, + {Path: "b.go", Line: 3, Body: "and here"}, + }, + }) + if err != nil { + t.Fatalf("SubmitReview: %v", err) + } + + req := rt.requests[0] + if req.Method != http.MethodPost { + t.Errorf("method = %s, want POST", req.Method) + } + if req.URL.String() != "https://api.github.com/repos/org/repo/pulls/13/reviews" { + t.Errorf("URL = %s", req.URL.String()) + } + if h := req.Header.Get("Authorization"); h != "Bearer sekret" { + t.Errorf("Authorization = %q", h) + } + wantBody := `{"event":"REQUEST_CHANGES","body":"please fix","comments":[{"path":"a.go","line":12,"side":"RIGHT","body":"here"},{"path":"b.go","line":3,"body":"and here"}]}` + if b := readReqBody(t, req); b != wantBody { + t.Errorf("request body = %q, want %q", b, wantBody) + } + if got.ID != 8801 || got.Verdict != "request_changes" || + got.URL != "https://github.com/org/repo/pull/13#pullrequestreview-8801" { + t.Errorf("decoded SubmittedReview = %+v", got) + } +} + +// verdict->event mapping for all three tokens. +func TestSubmitReviewEventMapping(t *testing.T) { + cases := map[string]string{ + "approve": "APPROVE", + "request_changes": "REQUEST_CHANGES", + "comment": "COMMENT", + } + for verdict, event := range cases { + t.Run(verdict, func(t *testing.T) { + rt := &scriptedRoundTripper{responses: []scriptedResponse{{status: 201, body: `{"id":1,"html_url":"u"}`}}} + g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) + if _, err := g.SubmitReview(context.Background(), "org/repo", 1, SubmitReview{Verdict: verdict, Body: "b"}); err != nil { + t.Fatalf("SubmitReview: %v", err) + } + wantBody := `{"event":"` + event + `","body":"b"}` + if b := readReqBody(t, rt.requests[0]); b != wantBody { + t.Errorf("request body = %q, want %q", b, wantBody) + } + }) + } +} + +// Empty comments omits the array entirely (not "comments":null). +func TestSubmitReviewEmptyCommentsOmitsArray(t *testing.T) { + rt := &scriptedRoundTripper{responses: []scriptedResponse{{status: 201, body: `{"id":1,"html_url":"u"}`}}} + g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) + if _, err := g.SubmitReview(context.Background(), "org/repo", 1, SubmitReview{Verdict: "comment", Body: "b"}); err != nil { + t.Fatalf("SubmitReview: %v", err) + } + wantBody := `{"event":"COMMENT","body":"b"}` + if b := readReqBody(t, rt.requests[0]); b != wantBody { + t.Errorf("request body = %q, want %q", b, wantBody) + } +} + +// An unknown verdict errors before any HTTP call. +func TestSubmitReviewUnknownVerdictNoWire(t *testing.T) { + rt := &scriptedRoundTripper{} + g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) + _, err := g.SubmitReview(context.Background(), "org/repo", 1, SubmitReview{Verdict: "lgtm", Body: "b"}) + if err == nil || !strings.Contains(err.Error(), "unknown verdict") { + t.Fatalf("err = %v, want unknown-verdict error", err) + } + if rt.calls != 0 { + t.Errorf("issued a request: calls = %d, want 0", rt.calls) + } +} + +// COMMENT and REQUEST_CHANGES with an empty body are rejected client-side with +// zero HTTP calls (GitHub requires a body for both). +func TestSubmitReviewEmptyBodyRejected(t *testing.T) { + for _, verdict := range []string{"comment", "request_changes"} { + t.Run(verdict, func(t *testing.T) { + rt := &scriptedRoundTripper{} + g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) + _, err := g.SubmitReview(context.Background(), "org/repo", 1, SubmitReview{Verdict: verdict}) + if err == nil || !strings.Contains(err.Error(), "requires a body") { + t.Fatalf("err = %v, want requires-a-body error", err) + } + if rt.calls != 0 { + t.Errorf("issued a request: calls = %d, want 0", rt.calls) + } + }) + } +} + +// APPROVE may be bodyless (A2): an empty-body approve succeeds. +func TestSubmitReviewApproveBodyless(t *testing.T) { + rt := &scriptedRoundTripper{responses: []scriptedResponse{{status: 201, body: `{"id":7,"html_url":"u"}`}}} + g := newTestGitHub(rt, &fakeTokenSource{token: "t"}) + got, err := g.SubmitReview(context.Background(), "org/repo", 1, SubmitReview{Verdict: "approve"}) + if err != nil { + t.Fatalf("SubmitReview: %v", err) + } + if b := readReqBody(t, rt.requests[0]); b != `{"event":"APPROVE","body":""}` { + t.Errorf("request body = %q", b) + } + if got.ID != 7 || got.Verdict != "approve" { + t.Errorf("decoded SubmittedReview = %+v", got) + } +} + +// An off-diff inline comment drives a mocked 422 -> *StatusError{422}. +func TestSubmitReviewOffDiff422(t *testing.T) { + rt := &scriptedRoundTripper{responses: []scriptedResponse{ + {status: 422, body: `{"message":"line must be part of the diff"}`}, + }} + ts := &fakeTokenSource{token: "t"} + g := newTestGitHub(rt, ts) + _, err := g.SubmitReview(context.Background(), "org/repo", 1, SubmitReview{ + Verdict: "comment", + Body: "b", + Comments: []ReviewCommentInput{{Path: "a.go", Line: 9999, Body: "off diff"}}, + }) + var se *StatusError + if !errors.As(err, &se) || se.Status != 422 { + t.Fatalf("err = %v, want *StatusError 422", err) + } + if ts.invalidated != 0 { + t.Errorf("422 must not Invalidate; got %d", ts.invalidated) + } +} + +// 403-rate / 403-bad-creds / 404 route through mapErrorResponse, like the other +// write methods. +func TestSubmitReviewErrorMapping(t *testing.T) { + t.Run("403 rate-limit -> ErrBudgetExhausted, no Invalidate", func(t *testing.T) { + rt := &scriptedRoundTripper{responses: []scriptedResponse{ + {status: 403, body: `{"message":"rate limited"}`, headers: map[string]string{"Retry-After": "60"}}, + }} + ts := &fakeTokenSource{token: "t"} + g := newTestGitHub(rt, ts) + _, err := g.SubmitReview(context.Background(), "org/repo", 1, SubmitReview{Verdict: "approve"}) + if !errors.Is(err, ErrBudgetExhausted) { + t.Fatalf("err = %v, want ErrBudgetExhausted", err) + } + if ts.invalidated != 0 { + t.Errorf("rate-limit must not Invalidate; got %d", ts.invalidated) + } + }) + t.Run("403 bad-creds -> StatusError + Invalidate", func(t *testing.T) { + rt := &scriptedRoundTripper{responses: []scriptedResponse{ + {status: 403, body: `{"message":"Bad credentials"}`}, + }} + ts := &fakeTokenSource{token: "t"} + g := newTestGitHub(rt, ts) + _, err := g.SubmitReview(context.Background(), "org/repo", 1, SubmitReview{Verdict: "approve"}) + var se *StatusError + if !errors.As(err, &se) || se.Status != 403 { + t.Fatalf("err = %v, want *StatusError 403", err) + } + if ts.invalidated != 1 { + t.Errorf("bad-creds must Invalidate; got %d", ts.invalidated) + } + }) + t.Run("404 -> StatusError, no Invalidate", func(t *testing.T) { + rt := &scriptedRoundTripper{responses: []scriptedResponse{ + {status: 404, body: `{"message":"Not Found"}`}, + }} + ts := &fakeTokenSource{token: "t"} + g := newTestGitHub(rt, ts) + _, err := g.SubmitReview(context.Background(), "org/repo", 1, SubmitReview{Verdict: "approve"}) + var se *StatusError + if !errors.As(err, &se) || se.Status != 404 { + t.Fatalf("err = %v, want *StatusError 404", err) + } + if ts.invalidated != 0 { + t.Errorf("404 must not Invalidate; got %d", ts.invalidated) + } + }) +} diff --git a/go/internal/forge/provider.go b/go/internal/forge/provider.go index dbb62e96..87d737ed 100644 --- a/go/internal/forge/provider.go +++ b/go/internal/forge/provider.go @@ -183,6 +183,29 @@ type CreatePR struct { Draft bool } +// SubmitReview is the input to Provider.SubmitReview. Body is PRE-stamp +// (the Service stamps it); Comments ride unstamped inside the stamped review. +type SubmitReview struct { + Verdict string // "approve" | "request_changes" | "comment" + Body string + Comments []ReviewCommentInput +} + +// ReviewCommentInput is one inline review comment carried inside a SubmitReview. +type ReviewCommentInput struct { + Path string + Line uint32 + Side string // "LEFT" | "RIGHT"; "" = RIGHT + Body string +} + +// SubmittedReview is the write ack a provider returns. +type SubmittedReview struct { + ID uint64 + URL string + Verdict string +} + // IssueFilter narrows Provider.ListIssues. type IssueFilter struct { // State selects by forge state ("open" | "closed" | "all"); empty means the @@ -197,7 +220,7 @@ type IssueFilter struct { // argument (the provider closes over its own). Body handling is the PROVIDER'S // contract: a Create/Comment method receives a body already stamped by the // Service, and a read method returns the body RAW — the Service strips/parses. -type Provider interface { +type Provider interface { //nolint:interfacebloat // one method per forge operation the Server drives; the surface is the forge contract, not incidental sprawl Name() string CreateIssue(ctx context.Context, repo string, in CreateIssue) (Issue, error) CommentOnIssue(ctx context.Context, repo string, number uint64, body string) (Comment, error) @@ -205,6 +228,10 @@ type Provider interface { ListIssues(ctx context.Context, repo string, f IssueFilter) ([]Issue, error) CreatePullRequest(ctx context.Context, repo string, in CreatePR) (PullRequest, error) CommentOnPullRequest(ctx context.Context, repo string, number uint64, body string) (Comment, error) + // SubmitReview submits a pull-request review (verdict + optional body + + // optional inline comments). in.Body is PRE-stamped by the Service. An + // unknown verdict is rejected before any wire call. + SubmitReview(ctx context.Context, repo string, number uint64, in SubmitReview) (SubmittedReview, error) GetPullRequest(ctx context.Context, repo string, number uint64) (PullRequest, error) // Checks returns the rolled-up CI/status state for a PR head. Separated from // GetPullRequest because the subscription poller needs it alone (#995 Decision 5). From 6736fc2181d71d15c464b6d0ae88eb694ef86e83 Mon Sep 17 00:00:00 2001 From: seal Date: Tue, 18 Aug 2026 22:52:43 -0400 Subject: [PATCH 2/2] fix(forge): unexport write-verdict consts + presize review comments (RIG-2208 review) Review low#1: the exported Verdict* consts were unspecced and name-collided with the read-side Review.Verdict field; make them package-private (verdictApprove/verdictRequestChanges/verdictComment) since nothing outside the package references them. Review low#2: presize the review-comments slice to len(in.Comments) to avoid the incremental-append reallocs. Co-authored-by: Matt Wilkinson --- go/internal/forge/github.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/go/internal/forge/github.go b/go/internal/forge/github.go index fc0ad017..e631276a 100644 --- a/go/internal/forge/github.go +++ b/go/internal/forge/github.go @@ -372,17 +372,19 @@ type reviewEvent struct { } // The write-side verdict vocabulary (design §T3): GitHub's reviews-POST event -// token set, distinct from the past-tense read-side Review.verdict. +// token set, distinct from the past-tense read-side Review.verdict. Package- +// private — the spec surfaces only the type triple and interface method; a +// caller passes the raw token, and T4's Service owns any exported vocabulary. const ( - VerdictApprove = "approve" - VerdictRequestChanges = "request_changes" - VerdictComment = "comment" + verdictApprove = "approve" + verdictRequestChanges = "request_changes" + verdictComment = "comment" ) var reviewEvents = map[string]reviewEvent{ - VerdictApprove: {token: "APPROVE", requiresBody: false}, - VerdictRequestChanges: {token: "REQUEST_CHANGES", requiresBody: true}, - VerdictComment: {token: "COMMENT", requiresBody: true}, + verdictApprove: {token: "APPROVE", requiresBody: false}, + verdictRequestChanges: {token: "REQUEST_CHANGES", requiresBody: true}, + verdictComment: {token: "COMMENT", requiresBody: true}, } // ghReviewComment is the wire shape of one inline comment inside a reviews POST. @@ -419,6 +421,7 @@ func (g *GitHub) SubmitReview(ctx context.Context, repo string, number uint64, i Body string `json:"body"` Comments []ghReviewComment `json:"comments,omitempty"` }{Event: ev.token, Body: in.Body} + body.Comments = make([]ghReviewComment, 0, len(in.Comments)) for _, c := range in.Comments { body.Comments = append(body.Comments, ghReviewComment(c)) }