Skip to content

Commit 7520adc

Browse files
feat: support multi-pr review runs
1 parent afe0817 commit 7520adc

20 files changed

Lines changed: 780 additions & 156 deletions

File tree

backend/internal/httpd/apispec/openapi.yaml

Lines changed: 49 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1243,13 +1243,13 @@ paths:
12431243
content:
12441244
application/json:
12451245
schema:
1246-
$ref: '#/components/schemas/ReviewRunResponse'
1246+
$ref: '#/components/schemas/TriggerReviewResponse'
12471247
description: OK
12481248
"201":
12491249
content:
12501250
application/json:
12511251
schema:
1252-
$ref: '#/components/schemas/ReviewRunResponse'
1252+
$ref: '#/components/schemas/TriggerReviewResponse'
12531253
description: Created
12541254
"404":
12551255
content:
@@ -1632,15 +1632,20 @@ components:
16321632
type: object
16331633
ListReviewsResponse:
16341634
properties:
1635+
reviewRuns:
1636+
items:
1637+
$ref: '#/components/schemas/ReviewRun'
1638+
type: array
16351639
reviewerHandleId:
16361640
type: string
16371641
reviews:
16381642
items:
1639-
$ref: '#/components/schemas/ReviewRun'
1643+
$ref: '#/components/schemas/PRReviewItem'
16401644
type: array
16411645
required:
16421646
- reviewerHandleId
16431647
- reviews
1648+
- reviewRuns
16441649
type: object
16451650
ListSessionPRsResponse:
16461651
properties:
@@ -1772,6 +1777,30 @@ components:
17721777
- id
17731778
- projectId
17741779
type: object
1780+
PRReviewItem:
1781+
properties:
1782+
latestRun:
1783+
$ref: '#/components/schemas/ReviewRun'
1784+
prNumber:
1785+
type: integer
1786+
prUrl:
1787+
type: string
1788+
status:
1789+
enum:
1790+
- needs_review
1791+
- running
1792+
- up_to_date
1793+
- changes_requested
1794+
- ineligible
1795+
type: string
1796+
targetSha:
1797+
type: string
1798+
required:
1799+
- prUrl
1800+
- prNumber
1801+
- targetSha
1802+
- status
1803+
type: object
17751804
Project:
17761805
properties:
17771806
agent:
@@ -2403,6 +2432,23 @@ components:
24032432
- body
24042433
- githubReviewId
24052434
type: object
2435+
TriggerReviewResponse:
2436+
properties:
2437+
createdRuns:
2438+
items:
2439+
$ref: '#/components/schemas/ReviewRun'
2440+
type: array
2441+
reviewerHandleId:
2442+
type: string
2443+
reviews:
2444+
items:
2445+
$ref: '#/components/schemas/PRReviewItem'
2446+
type: array
2447+
required:
2448+
- reviewerHandleId
2449+
- reviews
2450+
- createdRuns
2451+
type: object
24062452
WorkspaceRepo:
24072453
properties:
24082454
name:

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

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -181,11 +181,13 @@ var schemaNames = map[string]string{
181181
"ControllersResolveCommentsRequest": "ResolveCommentsRequest",
182182
"ControllersResolveCommentsResponse": "ResolveCommentsResponse",
183183
// httpd/controllers — review wire envelopes
184-
"ControllersListReviewsResponse": "ListReviewsResponse",
185-
"ControllersReviewRunResponse": "ReviewRunResponse",
186-
"ControllersSubmitReviewInput": "SubmitReviewInput",
184+
"ControllersListReviewsResponse": "ListReviewsResponse",
185+
"ControllersReviewRunResponse": "ReviewRunResponse",
186+
"ControllersTriggerReviewResponse": "TriggerReviewResponse",
187+
"ControllersSubmitReviewInput": "SubmitReviewInput",
187188
// domain review entities
188-
"DomainReviewRun": "ReviewRun",
189+
"DomainReviewRun": "ReviewRun",
190+
"ReviewPRReviewItem": "PRReviewItem",
189191
// service/project entities + DTOs
190192
"ProjectProject": "Project",
191193
"ProjectSummary": "ProjectSummary",
@@ -345,8 +347,8 @@ func reviewOperations() []operation {
345347
summary: "Trigger a code review of a worker's PR",
346348
pathParams: []any{controllers.SessionIDParam{}},
347349
resps: []respUnit{
348-
{http.StatusOK, controllers.ReviewRunResponse{}},
349-
{http.StatusCreated, controllers.ReviewRunResponse{}},
350+
{http.StatusOK, controllers.TriggerReviewResponse{}},
351+
{http.StatusCreated, controllers.TriggerReviewResponse{}},
350352
{http.StatusUnprocessableEntity, envelope.APIError{}},
351353
{http.StatusNotFound, envelope.APIError{}},
352354
{http.StatusNotImplemented, envelope.APIError{}},

backend/internal/httpd/controllers/reviews.go

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,24 +10,35 @@ import (
1010
"github.com/aoagents/agent-orchestrator/backend/internal/domain"
1111
"github.com/aoagents/agent-orchestrator/backend/internal/httpd/apispec"
1212
"github.com/aoagents/agent-orchestrator/backend/internal/httpd/envelope"
13+
reviewcore "github.com/aoagents/agent-orchestrator/backend/internal/review"
1314
reviewsvc "github.com/aoagents/agent-orchestrator/backend/internal/service/review"
1415
)
1516

1617
// ListReviewsResponse is the body of GET /api/v1/sessions/{sessionId}/reviews.
1718
// reviewerHandleId is the live reviewer pane's runtime handle, for the UI to
1819
// attach its terminal over /mux (empty when no reviewer has run).
1920
type ListReviewsResponse struct {
20-
ReviewerHandleID string `json:"reviewerHandleId"`
21-
Reviews []domain.ReviewRun `json:"reviews"`
21+
ReviewerHandleID string `json:"reviewerHandleId"`
22+
Reviews []reviewcore.PRReviewItem `json:"reviews"`
23+
ReviewRuns []domain.ReviewRun `json:"reviewRuns"`
2224
}
2325

24-
// ReviewRunResponse is the body of trigger (200/201) and submit (200). It
25-
// carries the run plus the reviewer pane handle so the UI can attach a terminal.
26+
// ReviewRunResponse is the body of submit (200). It carries the run plus the
27+
// reviewer pane handle so the UI can attach a terminal.
2628
type ReviewRunResponse struct {
2729
Review domain.ReviewRun `json:"review"`
2830
ReviewerHandleID string `json:"reviewerHandleId"`
2931
}
3032

33+
// TriggerReviewResponse is the body of trigger (200/201). reviews carries the
34+
// PR-scoped review state; createdRuns carries the new review_run rows started
35+
// by this trigger.
36+
type TriggerReviewResponse struct {
37+
ReviewerHandleID string `json:"reviewerHandleId"`
38+
Reviews []reviewcore.PRReviewItem `json:"reviews"`
39+
CreatedRuns []domain.ReviewRun `json:"createdRuns"`
40+
}
41+
3142
// SubmitReviewInput is the body of POST /api/v1/sessions/{sessionId}/reviews/submit.
3243
type SubmitReviewInput struct {
3344
RunID string `json:"runId" description:"Review run id being completed."`
@@ -62,7 +73,11 @@ func (c *ReviewsController) list(w http.ResponseWriter, r *http.Request) {
6273
if runs == nil {
6374
runs = []domain.ReviewRun{}
6475
}
65-
envelope.WriteJSON(w, http.StatusOK, ListReviewsResponse{ReviewerHandleID: res.ReviewerHandleID, Reviews: runs})
76+
items := res.Items
77+
if items == nil {
78+
items = []reviewcore.PRReviewItem{}
79+
}
80+
envelope.WriteJSON(w, http.StatusOK, ListReviewsResponse{ReviewerHandleID: res.ReviewerHandleID, Reviews: items, ReviewRuns: runs})
6681
}
6782

6883
func (c *ReviewsController) trigger(w http.ResponseWriter, r *http.Request) {
@@ -81,7 +96,19 @@ func (c *ReviewsController) trigger(w http.ResponseWriter, r *http.Request) {
8196
if res.Created {
8297
status = http.StatusCreated
8398
}
84-
envelope.WriteJSON(w, status, ReviewRunResponse{Review: res.Run, ReviewerHandleID: res.ReviewerHandleID})
99+
createdRuns := res.CreatedRuns
100+
if createdRuns == nil {
101+
createdRuns = []domain.ReviewRun{}
102+
}
103+
reviews := res.Items
104+
if reviews == nil {
105+
reviews = []reviewcore.PRReviewItem{}
106+
}
107+
envelope.WriteJSON(w, status, TriggerReviewResponse{
108+
ReviewerHandleID: res.ReviewerHandleID,
109+
Reviews: reviews,
110+
CreatedRuns: createdRuns,
111+
})
85112
}
86113

87114
func (c *ReviewsController) submit(w http.ResponseWriter, r *http.Request) {

backend/internal/httpd/controllers/reviews_test.go

Lines changed: 57 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,17 @@ import (
2020

2121
type fakeReviewService struct {
2222
triggerErr error
23+
trigger reviewcore.TriggerResult
24+
list reviewcore.SessionReviews
2325
}
2426

2527
func (f *fakeReviewService) Trigger(context.Context, domain.SessionID) (reviewcore.TriggerResult, error) {
2628
if f.triggerErr != nil {
2729
return reviewcore.TriggerResult{}, f.triggerErr
2830
}
31+
if f.trigger.ReviewerHandleID != "" || f.trigger.Run.ID != "" || f.trigger.Items != nil || f.trigger.CreatedRuns != nil {
32+
return f.trigger, nil
33+
}
2934
return reviewcore.TriggerResult{Run: domain.ReviewRun{ID: "run-1"}, Created: true}, nil
3035
}
3136

@@ -34,7 +39,7 @@ func (f *fakeReviewService) Submit(context.Context, domain.SessionID, string, do
3439
}
3540

3641
func (f *fakeReviewService) List(context.Context, domain.SessionID) (reviewcore.SessionReviews, error) {
37-
return reviewcore.SessionReviews{}, nil
42+
return f.list, nil
3843
}
3944

4045
func newReviewTestServer(t *testing.T, svc reviewsvc.Manager) *httptest.Server {
@@ -59,3 +64,54 @@ func TestReviewsTrigger_MissingReviewerBinaryReturns422WithCause(t *testing.T) {
5964
t.Fatalf("message = %q, want reviewer binary cause", got.Message)
6065
}
6166
}
67+
68+
func TestReviewsListIncludesItems(t *testing.T) {
69+
srv := newReviewTestServer(t, &fakeReviewService{list: reviewcore.SessionReviews{
70+
ReviewerHandleID: "review-mer-1",
71+
Runs: []domain.ReviewRun{{ID: "run-1", PRURL: "https://github.com/o/r/pull/1", TargetSHA: "sha1"}},
72+
Items: []reviewcore.PRReviewItem{{PRURL: "https://github.com/o/r/pull/1", PRNumber: 1, TargetSHA: "sha1", Status: reviewcore.ItemUpToDate}},
73+
}})
74+
75+
body, status, headers := doRequest(t, srv, "GET", "/api/v1/sessions/mer-1/reviews", "")
76+
assertJSON(t, headers)
77+
if status != http.StatusOK {
78+
t.Fatalf("status = %d body=%s", status, body)
79+
}
80+
if !strings.Contains(string(body), `"reviews"`) || !strings.Contains(string(body), `"reviewRuns"`) || !strings.Contains(string(body), `"up_to_date"`) || !strings.Contains(string(body), `"reviewerHandleId":"review-mer-1"`) {
81+
t.Fatalf("body missing review items/handle: %s", body)
82+
}
83+
if strings.Contains(string(body), `"items"`) || strings.Contains(string(body), `"reviewItems"`) {
84+
t.Fatalf("body contains deprecated review item aliases: %s", body)
85+
}
86+
}
87+
88+
func TestReviewsTriggerIncludesBatchFields(t *testing.T) {
89+
run1 := domain.ReviewRun{ID: "run-1", PRURL: "https://github.com/o/r/pull/1", TargetSHA: "sha1"}
90+
run2 := domain.ReviewRun{ID: "run-2", PRURL: "https://github.com/o/r/pull/2", TargetSHA: "sha2"}
91+
srv := newReviewTestServer(t, &fakeReviewService{trigger: reviewcore.TriggerResult{
92+
Run: run1,
93+
ReviewerHandleID: "review-mer-1",
94+
Created: true,
95+
CreatedRuns: []domain.ReviewRun{run1, run2},
96+
Items: []reviewcore.PRReviewItem{
97+
{PRURL: run1.PRURL, PRNumber: 1, TargetSHA: run1.TargetSHA, Status: reviewcore.ItemRunning, LatestRun: &run1},
98+
{PRURL: run2.PRURL, PRNumber: 2, TargetSHA: run2.TargetSHA, Status: reviewcore.ItemRunning, LatestRun: &run2},
99+
},
100+
}})
101+
102+
body, status, headers := doRequest(t, srv, "POST", "/api/v1/sessions/mer-1/reviews/trigger", "")
103+
assertJSON(t, headers)
104+
if status != http.StatusCreated {
105+
t.Fatalf("status = %d body=%s", status, body)
106+
}
107+
for _, want := range []string{`"reviews"`, `"createdRuns"`, `"running"`, `"run-1"`, `"run-2"`, `"reviewerHandleId":"review-mer-1"`} {
108+
if !strings.Contains(string(body), want) {
109+
t.Fatalf("body missing %s: %s", want, body)
110+
}
111+
}
112+
for _, unwanted := range []string{`"reviewItems"`, `"items"`, `"createdReviews"`, `"review":`} {
113+
if strings.Contains(string(body), unwanted) {
114+
t.Fatalf("body contains deprecated field %s: %s", unwanted, body)
115+
}
116+
}
117+
}

backend/internal/lifecycle/manager_test.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -543,8 +543,10 @@ func TestApplyReviewResultSendsAndDedupsThroughPRSignature(t *testing.T) {
543543
t.Fatalf("outcome/messages = %q/%v, want sent once", outcome, msg.msgs)
544544
}
545545
got := msg.msgs[0]
546-
if !strings.Contains(got, "[AO reviewer]") || !strings.Contains(got, "fix the bug") || !strings.Contains(got, "98[2J765") {
547-
t.Fatalf("AO review nudge missing label/body/review id: %q", got)
546+
for _, want := range []string{"[AO reviewer]", "PR: " + result.PRURL, "Verdict: changes_requested", "Review body:\nfix the bug", "GitHub review: 98[2J765"} {
547+
if !strings.Contains(got, want) {
548+
t.Fatalf("AO review nudge missing %q: %q", want, got)
549+
}
548550
}
549551
if strings.Contains(got, "\x1b") {
550552
t.Fatalf("AO review nudge should sanitize control bytes: %q", got)

backend/internal/lifecycle/reactions.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -154,13 +154,14 @@ func (m *Manager) ApplyReviewResult(ctx context.Context, workerID domain.Session
154154
if m.messenger == nil {
155155
return ReviewDeliveryNoop, nil
156156
}
157-
msg := "[AO reviewer] AO's internal code reviewer requested changes on your PR. Review the feedback below and address it."
157+
msg := fmt.Sprintf("[AO reviewer] AO's internal code reviewer submitted a review.\n\nPR: %s\nVerdict: %s", domain.SanitizeControlChars(r.PRURL), domain.SanitizeControlChars(string(r.Verdict)))
158158
if r.GithubReviewID != "" {
159159
safeReviewID := domain.SanitizeControlChars(r.GithubReviewID)
160-
msg += fmt.Sprintf(" This feedback is GitHub review %s. Once you have addressed it, reply on that review referencing id %s with how you addressed it, then resolve the review comment threads you addressed.", safeReviewID, safeReviewID)
160+
msg += fmt.Sprintf("\nGitHub review: %s", safeReviewID)
161+
msg += fmt.Sprintf("\n\nOnce you have addressed it, reply on GitHub review %s with how you addressed it, then resolve the review comment threads you addressed.", safeReviewID)
161162
}
162163
if r.Body != "" {
163-
msg += "\n\n" + domain.SanitizeControlChars(r.Body)
164+
msg += "\n\nReview body:\n" + domain.SanitizeControlChars(r.Body)
164165
}
165166
key := "review:" + r.PRURL + ":ao:" + r.RunID
166167
sig := strings.Join([]string{r.TargetSHA, r.RunID, r.GithubReviewID, r.Body}, "\x00")

backend/internal/ports/reviewer.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,12 @@ 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.
43+
ReviewQueue []ReviewTask
44+
// ReviewIndex is this invocation's zero-based position in ReviewQueue.
45+
ReviewIndex int
4046
// WorkspacePath is the worker's checkout the reviewer reads.
4147
WorkspacePath string
4248
// Prompt and SystemPrompt are the review instructions AO authored centrally,
@@ -48,6 +54,13 @@ type ReviewInvocation struct {
4854
SystemPrompt string
4955
}
5056

57+
// ReviewTask is one PR/run in a multi-PR review trigger queue.
58+
type ReviewTask struct {
59+
RunID string
60+
PRURL string
61+
TargetSHA string
62+
}
63+
5164
// ReviewCommandSpec is how to launch a reviewer: the argv and any extra env the
5265
// adapter needs. AO supplies the workspace and review-tracking env around it.
5366
type ReviewCommandSpec struct {

backend/internal/review/launcher.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@ type LaunchSpec struct {
3131
WorkspacePath string
3232
PRURL string
3333
TargetSHA string
34+
ReviewQueue []ports.ReviewTask
35+
ReviewIndex int
3436
}
3537

3638
// reviewerRuntime is the runtime surface the launcher needs: create a pane,
@@ -73,6 +75,8 @@ func (l *agentLauncher) invocation(spec LaunchSpec) ports.ReviewInvocation {
7375
WorkerSessionID: spec.WorkerID,
7476
PRURL: spec.PRURL,
7577
TargetSHA: spec.TargetSHA,
78+
ReviewQueue: spec.ReviewQueue,
79+
ReviewIndex: spec.ReviewIndex,
7680
WorkspacePath: spec.WorkspacePath,
7781
Prompt: prompt,
7882
SystemPrompt: systemPrompt,

0 commit comments

Comments
 (0)