Skip to content

Commit b45f263

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

20 files changed

Lines changed: 762 additions & 162 deletions

File tree

backend/internal/httpd/apispec/openapi.yaml

Lines changed: 39 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:
@@ -1636,7 +1636,7 @@ components:
16361636
type: string
16371637
reviews:
16381638
items:
1639-
$ref: '#/components/schemas/ReviewRun'
1639+
$ref: '#/components/schemas/PRReviewState'
16401640
type: array
16411641
required:
16421642
- reviewerHandleId
@@ -1772,6 +1772,30 @@ components:
17721772
- id
17731773
- projectId
17741774
type: object
1775+
PRReviewState:
1776+
properties:
1777+
latestRun:
1778+
$ref: '#/components/schemas/ReviewRun'
1779+
prNumber:
1780+
type: integer
1781+
prUrl:
1782+
type: string
1783+
status:
1784+
enum:
1785+
- needs_review
1786+
- running
1787+
- up_to_date
1788+
- changes_requested
1789+
- ineligible
1790+
type: string
1791+
targetSha:
1792+
type: string
1793+
required:
1794+
- prUrl
1795+
- prNumber
1796+
- targetSha
1797+
- status
1798+
type: object
17751799
Project:
17761800
properties:
17771801
agent:
@@ -2403,6 +2427,18 @@ components:
24032427
- body
24042428
- githubReviewId
24052429
type: object
2430+
TriggerReviewResponse:
2431+
properties:
2432+
reviewerHandleId:
2433+
type: string
2434+
reviews:
2435+
items:
2436+
$ref: '#/components/schemas/PRReviewState'
2437+
type: array
2438+
required:
2439+
- reviewerHandleId
2440+
- reviews
2441+
type: object
24062442
WorkspaceRepo:
24072443
properties:
24082444
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+
"ReviewPRReviewState": "PRReviewState",
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: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -10,24 +10,32 @@ 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.PRReviewState `json:"reviews"`
2223
}
2324

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.
25+
// ReviewRunResponse is the body of submit (200). It carries the run plus the
26+
// reviewer pane handle so the UI can attach a terminal.
2627
type ReviewRunResponse struct {
2728
Review domain.ReviewRun `json:"review"`
2829
ReviewerHandleID string `json:"reviewerHandleId"`
2930
}
3031

32+
// TriggerReviewResponse is the body of trigger (200/201). reviews carries the
33+
// PR-scoped review state after the trigger.
34+
type TriggerReviewResponse struct {
35+
ReviewerHandleID string `json:"reviewerHandleId"`
36+
Reviews []reviewcore.PRReviewState `json:"reviews"`
37+
}
38+
3139
// SubmitReviewInput is the body of POST /api/v1/sessions/{sessionId}/reviews/submit.
3240
type SubmitReviewInput struct {
3341
RunID string `json:"runId" description:"Review run id being completed."`
@@ -58,11 +66,11 @@ func (c *ReviewsController) list(w http.ResponseWriter, r *http.Request) {
5866
writeReviewError(w, r, err)
5967
return
6068
}
61-
runs := res.Runs
62-
if runs == nil {
63-
runs = []domain.ReviewRun{}
69+
reviews := res.Reviews
70+
if reviews == nil {
71+
reviews = []reviewcore.PRReviewState{}
6472
}
65-
envelope.WriteJSON(w, http.StatusOK, ListReviewsResponse{ReviewerHandleID: res.ReviewerHandleID, Reviews: runs})
73+
envelope.WriteJSON(w, http.StatusOK, ListReviewsResponse{ReviewerHandleID: res.ReviewerHandleID, Reviews: reviews})
6674
}
6775

6876
func (c *ReviewsController) trigger(w http.ResponseWriter, r *http.Request) {
@@ -81,7 +89,14 @@ func (c *ReviewsController) trigger(w http.ResponseWriter, r *http.Request) {
8189
if res.Created {
8290
status = http.StatusCreated
8391
}
84-
envelope.WriteJSON(w, status, ReviewRunResponse{Review: res.Run, ReviewerHandleID: res.ReviewerHandleID})
92+
reviews := res.Reviews
93+
if reviews == nil {
94+
reviews = []reviewcore.PRReviewState{}
95+
}
96+
envelope.WriteJSON(w, status, TriggerReviewResponse{
97+
ReviewerHandleID: res.ReviewerHandleID,
98+
Reviews: reviews,
99+
})
85100
}
86101

87102
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.Reviews != 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 TestReviewsListIncludesReviewStates(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+
Reviews: []reviewcore.PRReviewState{{PRURL: "https://github.com/o/r/pull/1", PRNumber: 1, TargetSHA: "sha1", Status: reviewcore.ReviewStateUpToDate}},
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), `"up_to_date"`) || !strings.Contains(string(body), `"reviewerHandleId":"review-mer-1"`) {
81+
t.Fatalf("body missing review states/handle: %s", body)
82+
}
83+
if strings.Contains(string(body), `"items"`) || strings.Contains(string(body), `"reviewItems"`) || strings.Contains(string(body), `"reviewRuns"`) {
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+
Reviews: []reviewcore.PRReviewState{
97+
{PRURL: run1.PRURL, PRNumber: 1, TargetSHA: run1.TargetSHA, Status: reviewcore.ReviewStateRunning, LatestRun: &run1},
98+
{PRURL: run2.PRURL, PRNumber: 2, TargetSHA: run2.TargetSHA, Status: reviewcore.ReviewStateRunning, 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"`, `"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"`, `"createdRuns"`, `"reviewRuns"`, `"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)