feat(forge): GitHub SubmitReview + Provider.SubmitReview method (RIG-2170)

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 {

go/internal/forge/github.go

@@ -362,6 +362,78 @@ 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. 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"
+)
+
+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}
+	body.Comments = make([]ghReviewComment, 0, len(in.Comments))
+	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

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)
+		}
+	})
+}

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,14 +220,18 @@ 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)
 	GetIssue(ctx context.Context, repo string, number uint64) (Issue, error)
 	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).