Skip to content

Commit 2be6ed1

Browse files
fix: submit multi-pr reviews as one batch
1 parent 0be7112 commit 2be6ed1

16 files changed

Lines changed: 392 additions & 174 deletions

File tree

backend/internal/cli/review.go

Lines changed: 68 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package cli
22

33
import (
4+
"encoding/json"
45
"errors"
56
"fmt"
67
"io"
@@ -31,16 +32,26 @@ type reviewRun struct {
3132

3233
// reviewRunResponse mirrors controllers.ReviewRunResponse.
3334
type reviewRunResponse struct {
34-
Review reviewRun `json:"review"`
35-
ReviewerHandleID string `json:"reviewerHandleId"`
35+
Review reviewRun `json:"review"`
36+
Reviews []reviewRun `json:"reviews"`
37+
ReviewerHandleID string `json:"reviewerHandleId"`
3638
}
3739

38-
// submitReviewRequest mirrors controllers.SubmitReviewInput.
39-
type submitReviewRequest struct {
40+
// submitReviewItem mirrors controllers.SubmitReviewItem.
41+
type submitReviewItem struct {
4042
RunID string `json:"runId"`
4143
Verdict string `json:"verdict"`
42-
Body string `json:"body"`
43-
GithubReviewID string `json:"githubReviewId"`
44+
Body string `json:"body,omitempty"`
45+
GithubReviewID string `json:"githubReviewId,omitempty"`
46+
}
47+
48+
// submitReviewRequest mirrors controllers.SubmitReviewInput.
49+
type submitReviewRequest struct {
50+
RunID string `json:"runId,omitempty"`
51+
Verdict string `json:"verdict,omitempty"`
52+
Body string `json:"body,omitempty"`
53+
GithubReviewID string `json:"githubReviewId,omitempty"`
54+
Reviews []submitReviewItem `json:"reviews,omitempty"`
4455
}
4556

4657
type reviewSubmitOptions struct {
@@ -49,6 +60,7 @@ type reviewSubmitOptions struct {
4960
verdict string
5061
body string
5162
reviewID string
63+
reviews string
5264
}
5365

5466
func newReviewCommand(ctx *commandContext) *cobra.Command {
@@ -80,6 +92,7 @@ func newReviewSubmitCommand(ctx *commandContext) *cobra.Command {
8092
cmd.Flags().StringVar(&opts.verdict, "verdict", "", "Review verdict: approved or changes_requested (required)")
8193
cmd.Flags().StringVar(&opts.body, "body", "", "Review body: a path to a Markdown file, or - to read from stdin (so nothing is written into the worktree)")
8294
cmd.Flags().StringVar(&opts.reviewID, "review-id", "", "Id of the GitHub PR review just posted (the .id from the gh api POST that created the review)")
95+
cmd.Flags().StringVar(&opts.reviews, "reviews", "", "JSON review results array or object: a path, or - to read from stdin")
8396
return cmd
8497
}
8598

@@ -91,6 +104,9 @@ func (c *commandContext) submitReview(cmd *cobra.Command, args []string, opts re
91104
if session == "" {
92105
return usageError{errors.New("usage: worker session id is required (positional or --session)")}
93106
}
107+
if strings.TrimSpace(opts.reviews) != "" {
108+
return c.submitReviewBatch(cmd, session, opts)
109+
}
94110
runID := strings.TrimSpace(opts.runID)
95111
if runID == "" {
96112
return usageError{errors.New("usage: --run is required")}
@@ -124,3 +140,49 @@ func (c *commandContext) submitReview(cmd *cobra.Command, args []string, opts re
124140
_, err := fmt.Fprintf(cmd.OutOrStdout(), "recorded %s review for %s\n", res.Review.Verdict, session)
125141
return err
126142
}
143+
144+
func (c *commandContext) submitReviewBatch(cmd *cobra.Command, session string, opts reviewSubmitOptions) error {
145+
if strings.TrimSpace(opts.runID) != "" || strings.TrimSpace(opts.verdict) != "" || strings.TrimSpace(opts.body) != "" || strings.TrimSpace(opts.reviewID) != "" {
146+
return usageError{errors.New("usage: --reviews cannot be combined with --run, --verdict, --body, or --review-id")}
147+
}
148+
reviews, err := readReviewItems(cmd, strings.TrimSpace(opts.reviews))
149+
if err != nil {
150+
return err
151+
}
152+
path := "sessions/" + url.PathEscape(session) + "/reviews/submit"
153+
var res reviewRunResponse
154+
if err := c.postJSON(cmd.Context(), path, submitReviewRequest{Reviews: reviews}, &res); err != nil {
155+
return err
156+
}
157+
count := len(res.Reviews)
158+
if count == 0 {
159+
count = len(reviews)
160+
}
161+
_, err = fmt.Fprintf(cmd.OutOrStdout(), "recorded %d review(s) for %s\n", count, session)
162+
return err
163+
}
164+
165+
func readReviewItems(cmd *cobra.Command, path string) ([]submitReviewItem, error) {
166+
var raw []byte
167+
var err error
168+
if path == "-" {
169+
raw, err = io.ReadAll(cmd.InOrStdin())
170+
} else {
171+
raw, err = os.ReadFile(path)
172+
}
173+
if err != nil {
174+
return nil, usageError{fmt.Errorf("read review results: %w", err)}
175+
}
176+
var req submitReviewRequest
177+
if err := json.Unmarshal(raw, &req); err == nil && len(req.Reviews) > 0 {
178+
return req.Reviews, nil
179+
}
180+
var reviews []submitReviewItem
181+
if err := json.Unmarshal(raw, &reviews); err != nil {
182+
return nil, usageError{fmt.Errorf("decode review results JSON: %w", err)}
183+
}
184+
if len(reviews) == 0 {
185+
return nil, usageError{errors.New("usage: --reviews requires at least one review result")}
186+
}
187+
return reviews, nil
188+
}

backend/internal/cli/review_test.go

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,32 @@ func TestReviewSubmitAcceptsUnderscoreFlags(t *testing.T) {
104104
}
105105
}
106106

107+
func TestReviewSubmitBatchReadsReviewsFromStdin(t *testing.T) {
108+
cfg := setConfigEnv(t)
109+
srv, capture := reviewServer(t, http.StatusOK, `{"reviews":[{"id":"run-1","verdict":"changes_requested"},{"id":"run-2","verdict":"approved"}]}`)
110+
writeRunFileFor(t, cfg, srv)
111+
112+
deps := aliveDeps()
113+
deps.In = strings.NewReader(`{"reviews":[{"runId":"run-1","verdict":"changes_requested","body":"fix auth","githubReviewId":"101"},{"runId":"run-2","verdict":"approved","body":"looks good"}]}`)
114+
out, errOut, err := executeCLI(t, deps, "review", "submit", "mer-1", "--reviews", "-")
115+
if err != nil {
116+
t.Fatalf("unexpected error: %v\nstderr=%s", err, errOut)
117+
}
118+
if !strings.Contains(out, "recorded 2 review(s) for mer-1") {
119+
t.Fatalf("stdout = %q", out)
120+
}
121+
var req submitReviewRequest
122+
if err := json.Unmarshal([]byte(capture.body), &req); err != nil {
123+
t.Fatalf("decode body: %v", err)
124+
}
125+
if len(req.Reviews) != 2 || req.Reviews[0].RunID != "run-1" || req.Reviews[0].GithubReviewID != "101" || req.Reviews[1].Verdict != "approved" {
126+
t.Fatalf("request = %+v", req)
127+
}
128+
if req.RunID != "" || req.Verdict != "" {
129+
t.Fatalf("batch request should not also set legacy fields: %+v", req)
130+
}
131+
}
132+
107133
func TestReviewSubmitUsesSessionFlag(t *testing.T) {
108134
cfg := setConfigEnv(t)
109135
srv, capture := reviewServer(t, http.StatusOK, `{"review":{"verdict":"approved"}}`)

backend/internal/httpd/apispec/openapi.yaml

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2007,8 +2007,13 @@ components:
20072007
$ref: '#/components/schemas/ReviewRun'
20082008
reviewerHandleId:
20092009
type: string
2010+
reviews:
2011+
items:
2012+
$ref: '#/components/schemas/ReviewRun'
2013+
type: array
20102014
required:
20112015
- review
2016+
- reviews
20122017
- reviewerHandleId
20132018
type: object
20142019
RoleOverride:
@@ -2411,6 +2416,26 @@ components:
24112416
- projectId
24122417
type: object
24132418
SubmitReviewInput:
2419+
properties:
2420+
body:
2421+
description: Review body recorded by AO. Required for changes_requested.
2422+
type: string
2423+
githubReviewId:
2424+
description: Id of the GitHub PR review the reviewer posted, if any.
2425+
type: string
2426+
reviews:
2427+
description: Batched review results recorded by one reviewer CLI command.
2428+
items:
2429+
$ref: '#/components/schemas/SubmitReviewItem'
2430+
type: array
2431+
runId:
2432+
description: Review run id being completed.
2433+
type: string
2434+
verdict:
2435+
description: 'Review verdict: approved or changes_requested.'
2436+
type: string
2437+
type: object
2438+
SubmitReviewItem:
24142439
properties:
24152440
body:
24162441
description: Review body recorded by AO. Required for changes_requested.
@@ -2427,8 +2452,6 @@ components:
24272452
required:
24282453
- runId
24292454
- verdict
2430-
- body
2431-
- githubReviewId
24322455
type: object
24332456
TriggerReviewResponse:
24342457
properties:

backend/internal/httpd/apispec/specgen/build.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,7 @@ var schemaNames = map[string]string{
184184
"ControllersListReviewsResponse": "ListReviewsResponse",
185185
"ControllersReviewRunResponse": "ReviewRunResponse",
186186
"ControllersTriggerReviewResponse": "TriggerReviewResponse",
187+
"ControllersSubmitReviewItem": "SubmitReviewItem",
187188
"ControllersSubmitReviewInput": "SubmitReviewInput",
188189
// domain review entities
189190
"DomainReviewRun": "ReviewRun",

backend/internal/httpd/controllers/reviews.go

Lines changed: 40 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,9 @@ type ListReviewsResponse struct {
2525
// ReviewRunResponse is the body of submit (200). It carries the run plus the
2626
// reviewer pane handle so the UI can attach a terminal.
2727
type ReviewRunResponse struct {
28-
Review domain.ReviewRun `json:"review"`
29-
ReviewerHandleID string `json:"reviewerHandleId"`
28+
Review domain.ReviewRun `json:"review"`
29+
Reviews []domain.ReviewRun `json:"reviews"`
30+
ReviewerHandleID string `json:"reviewerHandleId"`
3031
}
3132

3233
// TriggerReviewResponse is the body of trigger (200/201). reviews carries the
@@ -36,12 +37,21 @@ type TriggerReviewResponse struct {
3637
Reviews []reviewcore.PRReviewState `json:"reviews"`
3738
}
3839

39-
// SubmitReviewInput is the body of POST /api/v1/sessions/{sessionId}/reviews/submit.
40-
type SubmitReviewInput struct {
40+
// SubmitReviewItem is one review result in a batched submit request.
41+
type SubmitReviewItem struct {
4142
RunID string `json:"runId" description:"Review run id being completed."`
4243
Verdict string `json:"verdict" description:"Review verdict: approved or changes_requested."`
43-
Body string `json:"body" description:"Review body recorded by AO. Required for changes_requested."`
44-
GithubReviewID string `json:"githubReviewId" description:"Id of the GitHub PR review the reviewer posted, if any."`
44+
Body string `json:"body,omitempty" description:"Review body recorded by AO. Required for changes_requested."`
45+
GithubReviewID string `json:"githubReviewId,omitempty" description:"Id of the GitHub PR review the reviewer posted, if any."`
46+
}
47+
48+
// SubmitReviewInput is the body of POST /api/v1/sessions/{sessionId}/reviews/submit.
49+
type SubmitReviewInput struct {
50+
RunID string `json:"runId,omitempty" description:"Review run id being completed."`
51+
Verdict string `json:"verdict,omitempty" description:"Review verdict: approved or changes_requested."`
52+
Body string `json:"body,omitempty" description:"Review body recorded by AO. Required for changes_requested."`
53+
GithubReviewID string `json:"githubReviewId,omitempty" description:"Id of the GitHub PR review the reviewer posted, if any."`
54+
Reviews []SubmitReviewItem `json:"reviews,omitempty" description:"Batched review results recorded by one reviewer CLI command."`
4555
}
4656

4757
// ReviewsController owns the session-scoped /reviews routes. A nil Svc returns 501.
@@ -109,12 +119,34 @@ func (c *ReviewsController) submit(w http.ResponseWriter, r *http.Request) {
109119
envelope.WriteAPIError(w, r, http.StatusBadRequest, "bad_request", "INVALID_BODY", "Invalid request body", nil)
110120
return
111121
}
112-
run, err := c.Svc.Submit(r.Context(), sessionID(r), in.RunID, domain.ReviewVerdict(in.Verdict), in.Body, in.GithubReviewID)
122+
reviews := make([]reviewsvc.SubmittedReview, 0, len(in.Reviews))
123+
if len(in.Reviews) > 0 {
124+
for _, item := range in.Reviews {
125+
reviews = append(reviews, reviewsvc.SubmittedReview{
126+
RunID: item.RunID,
127+
Verdict: domain.ReviewVerdict(item.Verdict),
128+
Body: item.Body,
129+
GithubReviewID: item.GithubReviewID,
130+
})
131+
}
132+
} else {
133+
reviews = append(reviews, reviewsvc.SubmittedReview{
134+
RunID: in.RunID,
135+
Verdict: domain.ReviewVerdict(in.Verdict),
136+
Body: in.Body,
137+
GithubReviewID: in.GithubReviewID,
138+
})
139+
}
140+
runs, err := c.Svc.SubmitMany(r.Context(), sessionID(r), reviews)
113141
if err != nil {
114142
writeReviewError(w, r, err)
115143
return
116144
}
117-
envelope.WriteJSON(w, http.StatusOK, ReviewRunResponse{Review: run})
145+
first := domain.ReviewRun{}
146+
if len(runs) > 0 {
147+
first = runs[0]
148+
}
149+
envelope.WriteJSON(w, http.StatusOK, ReviewRunResponse{Review: first, Reviews: runs})
118150
}
119151

120152
func writeReviewError(w http.ResponseWriter, r *http.Request, err error) {

backend/internal/httpd/controllers/reviews_test.go

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ type fakeReviewService struct {
2222
triggerErr error
2323
trigger reviewcore.TriggerResult
2424
list reviewcore.SessionReviews
25+
submitted []reviewsvc.SubmittedReview
2526
}
2627

2728
func (f *fakeReviewService) Trigger(context.Context, domain.SessionID) (reviewcore.TriggerResult, error) {
@@ -38,6 +39,15 @@ func (f *fakeReviewService) Submit(context.Context, domain.SessionID, string, do
3839
return domain.ReviewRun{}, nil
3940
}
4041

42+
func (f *fakeReviewService) SubmitMany(_ context.Context, _ domain.SessionID, reviews []reviewsvc.SubmittedReview) ([]domain.ReviewRun, error) {
43+
f.submitted = append([]reviewsvc.SubmittedReview(nil), reviews...)
44+
runs := make([]domain.ReviewRun, 0, len(reviews))
45+
for _, review := range reviews {
46+
runs = append(runs, domain.ReviewRun{ID: review.RunID, Verdict: review.Verdict, Body: review.Body, GithubReviewID: review.GithubReviewID})
47+
}
48+
return runs, nil
49+
}
50+
4151
func (f *fakeReviewService) List(context.Context, domain.SessionID) (reviewcore.SessionReviews, error) {
4252
return f.list, nil
4353
}
@@ -115,3 +125,22 @@ func TestReviewsTriggerIncludesBatchFields(t *testing.T) {
115125
}
116126
}
117127
}
128+
129+
func TestReviewsSubmitAcceptsBatchedReviews(t *testing.T) {
130+
svc := &fakeReviewService{}
131+
srv := newReviewTestServer(t, svc)
132+
133+
body, status, headers := doRequest(t, srv, "POST", "/api/v1/sessions/mer-1/reviews/submit", `{"reviews":[{"runId":"run-1","verdict":"changes_requested","body":"fix auth","githubReviewId":"101"},{"runId":"run-2","verdict":"approved"}]}`)
134+
assertJSON(t, headers)
135+
if status != http.StatusOK {
136+
t.Fatalf("status = %d body=%s", status, body)
137+
}
138+
if len(svc.submitted) != 2 || svc.submitted[0].RunID != "run-1" || svc.submitted[1].Verdict != domain.VerdictApproved {
139+
t.Fatalf("submitted = %+v", svc.submitted)
140+
}
141+
for _, want := range []string{`"reviews"`, `"run-1"`, `"run-2"`} {
142+
if !strings.Contains(string(body), want) {
143+
t.Fatalf("body missing %s: %s", want, body)
144+
}
145+
}
146+
}

backend/internal/lifecycle/reactions.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,9 @@ type ReviewResult struct {
4444
DeliveredAt *time.Time
4545
}
4646

47-
// ApplyReviewBatch reacts to a completed trigger batch after the review service
48-
// has decided which current-head changes-requested results are deliverable.
47+
// ApplyReviewBatch reacts to one reviewer CLI submission after the review
48+
// service has decided which current-head changes-requested results are
49+
// deliverable.
4950
func (m *Manager) ApplyReviewBatch(ctx context.Context, workerID domain.SessionID, batchID string, results []ReviewResult) (ReviewDeliveryOutcome, error) {
5051
if batchID == "" || len(results) == 0 {
5152
return ReviewDeliveryNoop, nil

backend/internal/ports/reviewer.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,9 +37,8 @@ type ReviewInvocation struct {
3737
PRURL string
3838
// TargetSHA is the PR head commit under review.
3939
TargetSHA string
40-
// ReviewQueue lists all review tasks created by the same trigger. Reviewers
41-
// still complete one RunID at a time, but this gives a shared reviewer pane
42-
// enough context to process a multi-PR queue in order.
40+
// ReviewQueue lists all review tasks created by the same trigger so a shared
41+
// reviewer pane can review multiple PRs and submit the results together.
4342
ReviewQueue []ReviewTask
4443
// ReviewIndex is this invocation's zero-based position in ReviewQueue.
4544
ReviewIndex int

0 commit comments

Comments
 (0)