Skip to content

Commit 6bc1a7c

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

16 files changed

Lines changed: 673 additions & 149 deletions

File tree

backend/internal/httpd/apispec/openapi.yaml

Lines changed: 56 additions & 2 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,6 +1632,10 @@ components:
16321632
type: object
16331633
ListReviewsResponse:
16341634
properties:
1635+
items:
1636+
items:
1637+
$ref: '#/components/schemas/ReviewPlanItem'
1638+
type: array
16351639
reviewerHandleId:
16361640
type: string
16371641
reviews:
@@ -1641,6 +1645,7 @@ components:
16411645
required:
16421646
- reviewerHandleId
16431647
- reviews
1648+
- items
16441649
type: object
16451650
ListSessionPRsResponse:
16461651
properties:
@@ -1931,6 +1936,30 @@ components:
19311936
- sessionId
19321937
- session
19331938
type: object
1939+
ReviewPlanItem:
1940+
properties:
1941+
latestRun:
1942+
$ref: '#/components/schemas/ReviewRun'
1943+
prNumber:
1944+
type: integer
1945+
prUrl:
1946+
type: string
1947+
status:
1948+
enum:
1949+
- needs_review
1950+
- running
1951+
- up_to_date
1952+
- changes_requested
1953+
- ineligible
1954+
type: string
1955+
targetSha:
1956+
type: string
1957+
required:
1958+
- prUrl
1959+
- prNumber
1960+
- targetSha
1961+
- status
1962+
type: object
19341963
ReviewRun:
19351964
properties:
19361965
body:
@@ -2403,6 +2432,31 @@ components:
24032432
- body
24042433
- githubReviewId
24052434
type: object
2435+
TriggerReviewResponse:
2436+
properties:
2437+
createdReviews:
2438+
items:
2439+
$ref: '#/components/schemas/ReviewRun'
2440+
type: array
2441+
items:
2442+
items:
2443+
$ref: '#/components/schemas/ReviewPlanItem'
2444+
type: array
2445+
review:
2446+
$ref: '#/components/schemas/ReviewRun'
2447+
reviewerHandleId:
2448+
type: string
2449+
reviews:
2450+
items:
2451+
$ref: '#/components/schemas/ReviewRun'
2452+
type: array
2453+
required:
2454+
- review
2455+
- reviewerHandleId
2456+
- reviews
2457+
- items
2458+
- createdReviews
2459+
type: object
24062460
WorkspaceRepo:
24072461
properties:
24082462
name:

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

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -181,9 +181,10 @@ 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
188189
"DomainReviewRun": "ReviewRun",
189190
// service/project entities + DTOs
@@ -345,8 +346,8 @@ func reviewOperations() []operation {
345346
summary: "Trigger a code review of a worker's PR",
346347
pathParams: []any{controllers.SessionIDParam{}},
347348
resps: []respUnit{
348-
{http.StatusOK, controllers.ReviewRunResponse{}},
349-
{http.StatusCreated, controllers.ReviewRunResponse{}},
349+
{http.StatusOK, controllers.TriggerReviewResponse{}},
350+
{http.StatusCreated, controllers.TriggerReviewResponse{}},
350351
{http.StatusUnprocessableEntity, envelope.APIError{}},
351352
{http.StatusNotFound, envelope.APIError{}},
352353
{http.StatusNotImplemented, envelope.APIError{}},

backend/internal/httpd/controllers/reviews.go

Lines changed: 37 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,24 +10,37 @@ 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 []domain.ReviewRun `json:"reviews"`
23+
Items []reviewcore.PlanItem `json:"items"`
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). review and
34+
// reviewerHandleId are legacy compatibility fields; items and createdReviews
35+
// carry the PR-scoped batch result.
36+
type TriggerReviewResponse struct {
37+
Review domain.ReviewRun `json:"review"`
38+
ReviewerHandleID string `json:"reviewerHandleId"`
39+
Reviews []domain.ReviewRun `json:"reviews"`
40+
Items []reviewcore.PlanItem `json:"items"`
41+
CreatedReviews []domain.ReviewRun `json:"createdReviews"`
42+
}
43+
3144
// SubmitReviewInput is the body of POST /api/v1/sessions/{sessionId}/reviews/submit.
3245
type SubmitReviewInput struct {
3346
RunID string `json:"runId" description:"Review run id being completed."`
@@ -62,7 +75,11 @@ func (c *ReviewsController) list(w http.ResponseWriter, r *http.Request) {
6275
if runs == nil {
6376
runs = []domain.ReviewRun{}
6477
}
65-
envelope.WriteJSON(w, http.StatusOK, ListReviewsResponse{ReviewerHandleID: res.ReviewerHandleID, Reviews: runs})
78+
items := res.Items
79+
if items == nil {
80+
items = []reviewcore.PlanItem{}
81+
}
82+
envelope.WriteJSON(w, http.StatusOK, ListReviewsResponse{ReviewerHandleID: res.ReviewerHandleID, Reviews: runs, Items: items})
6683
}
6784

6885
func (c *ReviewsController) trigger(w http.ResponseWriter, r *http.Request) {
@@ -81,7 +98,21 @@ func (c *ReviewsController) trigger(w http.ResponseWriter, r *http.Request) {
8198
if res.Created {
8299
status = http.StatusCreated
83100
}
84-
envelope.WriteJSON(w, status, ReviewRunResponse{Review: res.Run, ReviewerHandleID: res.ReviewerHandleID})
101+
reviews := res.CreatedRuns
102+
if reviews == nil {
103+
reviews = []domain.ReviewRun{}
104+
}
105+
items := res.Items
106+
if items == nil {
107+
items = []reviewcore.PlanItem{}
108+
}
109+
envelope.WriteJSON(w, status, TriggerReviewResponse{
110+
Review: res.Run,
111+
ReviewerHandleID: res.ReviewerHandleID,
112+
Reviews: reviews,
113+
Items: items,
114+
CreatedReviews: reviews,
115+
})
85116
}
86117

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

backend/internal/httpd/controllers/reviews_test.go

Lines changed: 49 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,46 @@ 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.PlanItem{{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), `"items"`) || !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+
}
84+
85+
func TestReviewsTriggerIncludesBatchFields(t *testing.T) {
86+
run1 := domain.ReviewRun{ID: "run-1", PRURL: "https://github.com/o/r/pull/1", TargetSHA: "sha1"}
87+
run2 := domain.ReviewRun{ID: "run-2", PRURL: "https://github.com/o/r/pull/2", TargetSHA: "sha2"}
88+
srv := newReviewTestServer(t, &fakeReviewService{trigger: reviewcore.TriggerResult{
89+
Run: run1,
90+
ReviewerHandleID: "review-mer-1",
91+
Created: true,
92+
CreatedRuns: []domain.ReviewRun{run1, run2},
93+
Items: []reviewcore.PlanItem{
94+
{PRURL: run1.PRURL, PRNumber: 1, TargetSHA: run1.TargetSHA, Status: reviewcore.ItemRunning, LatestRun: &run1},
95+
{PRURL: run2.PRURL, PRNumber: 2, TargetSHA: run2.TargetSHA, Status: reviewcore.ItemRunning, LatestRun: &run2},
96+
},
97+
}})
98+
99+
body, status, headers := doRequest(t, srv, "POST", "/api/v1/sessions/mer-1/reviews/trigger", "")
100+
assertJSON(t, headers)
101+
if status != http.StatusCreated {
102+
t.Fatalf("status = %d body=%s", status, body)
103+
}
104+
for _, want := range []string{`"createdReviews"`, `"reviews"`, `"items"`, `"run-1"`, `"run-2"`, `"reviewerHandleId":"review-mer-1"`} {
105+
if !strings.Contains(string(body), want) {
106+
t.Fatalf("body missing %s: %s", want, body)
107+
}
108+
}
109+
}

backend/internal/lifecycle/manager_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -543,8 +543,8 @@ 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+
if !strings.Contains(got, "[AO reviewer]") || !strings.Contains(got, result.PRURL) || !strings.Contains(got, "fix the bug") || !strings.Contains(got, "98[2J765") {
547+
t.Fatalf("AO review nudge missing label/pr url/body/review id: %q", got)
548548
}
549549
if strings.Contains(got, "\x1b") {
550550
t.Fatalf("AO review nudge should sanitize control bytes: %q", got)

backend/internal/lifecycle/reactions.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ 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 := "[AO reviewer] AO's internal code reviewer requested changes on your PR " + domain.SanitizeControlChars(r.PRURL) + ". Review the feedback below and address it."
158158
if r.GithubReviewID != "" {
159159
safeReviewID := domain.SanitizeControlChars(r.GithubReviewID)
160160
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)

backend/internal/review/planner.go

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
package review
2+
3+
import (
4+
"sort"
5+
6+
"github.com/aoagents/agent-orchestrator/backend/internal/domain"
7+
)
8+
9+
// ItemStatus is the per-PR review planning state.
10+
type ItemStatus string
11+
12+
const (
13+
// ItemNeedsReview means an eligible PR has no current AO approval or running pass.
14+
ItemNeedsReview ItemStatus = "needs_review"
15+
// ItemRunning means a review run is already active for the PR's current head.
16+
ItemRunning ItemStatus = "running"
17+
// ItemUpToDate means AO approved the PR's current head.
18+
ItemUpToDate ItemStatus = "up_to_date"
19+
// ItemChangesRequested means AO requested changes on the PR's current head.
20+
ItemChangesRequested ItemStatus = "changes_requested"
21+
// ItemIneligible means the PR is draft, closed, merged, or missing required facts.
22+
ItemIneligible ItemStatus = "ineligible"
23+
)
24+
25+
// PlanItem is one PR-scoped review decision for a worker session.
26+
type PlanItem struct {
27+
PRURL string `json:"prUrl"`
28+
PRNumber int `json:"prNumber"`
29+
TargetSHA string `json:"targetSha"`
30+
Status ItemStatus `json:"status" enum:"needs_review,running,up_to_date,changes_requested,ineligible"`
31+
LatestRun *domain.ReviewRun `json:"latestRun,omitempty"`
32+
}
33+
34+
// Plan computes per-PR review work from the currently observed PRs and existing
35+
// review runs. It is pure so the trigger path and API list path share exactly
36+
// the same eligibility/status rules.
37+
func Plan(prs []domain.PullRequest, runs []domain.ReviewRun) []PlanItem {
38+
latest := latestRunsByPRAndSHA(runs)
39+
items := make([]PlanItem, 0, len(prs))
40+
for _, pr := range prs {
41+
item := PlanItem{
42+
PRURL: pr.URL,
43+
PRNumber: pr.Number,
44+
TargetSHA: pr.HeadSHA,
45+
Status: ItemNeedsReview,
46+
}
47+
if pr.URL == "" || pr.HeadSHA == "" || pr.Draft || pr.Merged || pr.Closed {
48+
item.Status = ItemIneligible
49+
if run, ok := latest[item.PRURL+"\x00"+item.TargetSHA]; ok {
50+
item.LatestRun = &run
51+
}
52+
items = append(items, item)
53+
continue
54+
}
55+
if run, ok := latest[item.PRURL+"\x00"+item.TargetSHA]; ok {
56+
item.LatestRun = &run
57+
switch {
58+
case run.Status == domain.ReviewRunRunning:
59+
item.Status = ItemRunning
60+
case run.Verdict == domain.VerdictApproved:
61+
item.Status = ItemUpToDate
62+
case run.Verdict == domain.VerdictChangesRequested:
63+
item.Status = ItemChangesRequested
64+
case run.Status == domain.ReviewRunFailed:
65+
item.Status = ItemNeedsReview
66+
default:
67+
item.Status = ItemNeedsReview
68+
}
69+
}
70+
items = append(items, item)
71+
}
72+
sort.SliceStable(items, func(i, j int) bool {
73+
if items[i].PRNumber != items[j].PRNumber {
74+
return items[i].PRNumber < items[j].PRNumber
75+
}
76+
return items[i].PRURL < items[j].PRURL
77+
})
78+
return items
79+
}
80+
81+
func latestRunsByPRAndSHA(runs []domain.ReviewRun) map[string]domain.ReviewRun {
82+
latest := make(map[string]domain.ReviewRun)
83+
for _, run := range runs {
84+
if run.PRURL == "" || run.TargetSHA == "" {
85+
continue
86+
}
87+
key := run.PRURL + "\x00" + run.TargetSHA
88+
if existing, ok := latest[key]; !ok || run.CreatedAt.After(existing.CreatedAt) {
89+
latest[key] = run
90+
}
91+
}
92+
return latest
93+
}

0 commit comments

Comments
 (0)